From abe4ca6bcaf31e57a2a1dd3833660c896ee74379 Mon Sep 17 00:00:00 2001 From: David mattei Date: Fri, 12 Jul 2024 11:10:47 +0200 Subject: [PATCH 1/3] fix(core/revision): collection enter key triggers delete first item (#954) --- .../src/Form/DataField/CollectionItemFieldType.php | 9 --------- .../src/Resources/views/form/fields.html.twig | 6 ++++-- EMS/core-bundle/src/Service/DataService.php | 2 +- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/EMS/core-bundle/src/Form/DataField/CollectionItemFieldType.php b/EMS/core-bundle/src/Form/DataField/CollectionItemFieldType.php index dd350525f..6dda88dd5 100644 --- a/EMS/core-bundle/src/Form/DataField/CollectionItemFieldType.php +++ b/EMS/core-bundle/src/Form/DataField/CollectionItemFieldType.php @@ -6,7 +6,6 @@ use EMS\CoreBundle\Entity\FieldType; use EMS\CoreBundle\Form\DataTransformer\DataFieldModelTransformer; use EMS\CoreBundle\Form\DataTransformer\DataFieldViewTransformer; -use EMS\CoreBundle\Form\Field\SubmitEmsType; use Symfony\Component\Form\Extension\Core\Type\HiddenType; use Symfony\Component\Form\FormBuilderInterface; @@ -80,14 +79,6 @@ public function buildForm(FormBuilderInterface $builder, array $options): void ->addModelTransformer(new DataFieldModelTransformer($fieldType, $this->formRegistry)); } } - - $builder->add('remove_collection_item', SubmitEmsType::class, [ - 'attr' => [ - 'class' => 'btn btn-danger btn-sm remove-content-button', - ], - 'label' => 'Remove', - 'icon' => 'fa fa-trash', - ]); } /** diff --git a/EMS/core-bundle/src/Resources/views/form/fields.html.twig b/EMS/core-bundle/src/Resources/views/form/fields.html.twig index 1cd62c6e4..0d65c05f2 100644 --- a/EMS/core-bundle/src/Resources/views/form/fields.html.twig +++ b/EMS/core-bundle/src/Resources/views/form/fields.html.twig @@ -616,7 +616,9 @@
{{ form_label(form) }}
- {{- form_widget(form.remove_collection_item) -}} + {% if collapsible %}
@@ -631,7 +633,7 @@
- {% for child in form.iterator|filter(c => c.vars.name not in ['remove_collection_item', '_ems_internal_deleted']) %} + {% for child in form.iterator|filter(c => c.vars.name not in ['_ems_internal_deleted']) %}
{{- form_row(child) -}}
diff --git a/EMS/core-bundle/src/Service/DataService.php b/EMS/core-bundle/src/Service/DataService.php index 1561124be..cd57d7424 100644 --- a/EMS/core-bundle/src/Service/DataService.php +++ b/EMS/core-bundle/src/Service/DataService.php @@ -405,7 +405,7 @@ public function convertInputValues(DataField $dataField): void public static function isInternalField(string $fieldName): bool { - return \in_array($fieldName, ['_ems_internal_deleted', 'remove_collection_item']); + return '_ems_internal_deleted' === $fieldName; } public function generateInputValues(DataField $dataField): void From bf017adaf97b6d0020c00c49646a1a9a4f0d27b2 Mon Sep 17 00:00:00 2001 From: David mattei Date: Fri, 12 Jul 2024 19:44:42 +0200 Subject: [PATCH 2/3] fix(core/revision): duplicate ouuid put back deleted revision (#955) --- .../ContentManagement/CrudController.php | 20 ++--- .../Controller/Revision/TrashController.php | 14 ++-- .../src/Exception/DuplicateOuuidException.php | 8 ++ .../src/Repository/RevisionRepository.php | 41 ++++----- EMS/core-bundle/src/Service/DataService.php | 84 +++++++++---------- 5 files changed, 90 insertions(+), 77 deletions(-) diff --git a/EMS/core-bundle/src/Controller/ContentManagement/CrudController.php b/EMS/core-bundle/src/Controller/ContentManagement/CrudController.php index 508190bf2..dbaf68c0e 100644 --- a/EMS/core-bundle/src/Controller/ContentManagement/CrudController.php +++ b/EMS/core-bundle/src/Controller/ContentManagement/CrudController.php @@ -45,20 +45,20 @@ public function createAction(?string $ouuid, string $name, Request $request): Re $this->dataService->refresh($newRevision->giveContentType()->giveEnvironment()); } } catch (\Exception $e) { - if (($e instanceof NotFoundHttpException) or ($e instanceof BadRequestHttpException)) { + if ($e instanceof NotFoundHttpException || $e instanceof BadRequestHttpException) { throw $e; - } else { - $this->logger->error('log.crud.create_error', [ - EmsFields::LOG_CONTENTTYPE_FIELD => $contentType->getName(), - EmsFields::LOG_ERROR_MESSAGE_FIELD => $e->getMessage(), - EmsFields::LOG_EXCEPTION_FIELD => $e, - ]); } + $this->logger->error('log.crud.create_error', [ + EmsFields::LOG_CONTENTTYPE_FIELD => $contentType->getName(), + EmsFields::LOG_ERROR_MESSAGE_FIELD => $e->getMessage(), + EmsFields::LOG_EXCEPTION_FIELD => $e, + ]); + return $this->render("@$this->templateNamespace/ajax/notification.json.twig", [ - 'success' => false, - 'ouuid' => $ouuid, - 'type' => $contentType->getName(), + 'success' => false, + 'ouuid' => $ouuid, + 'type' => $contentType->getName(), ]); } diff --git a/EMS/core-bundle/src/Controller/Revision/TrashController.php b/EMS/core-bundle/src/Controller/Revision/TrashController.php index 50db221dc..53e886aba 100644 --- a/EMS/core-bundle/src/Controller/Revision/TrashController.php +++ b/EMS/core-bundle/src/Controller/Revision/TrashController.php @@ -81,9 +81,13 @@ public function putBack(ContentType $contentType, string $ouuid): RedirectRespon throw $this->createAccessDeniedException(); } - $revisionId = $this->dataService->trashPutBack($contentType, $ouuid); + $restoredRevision = $this->dataService->trashPutBackAsDraft($contentType, $ouuid); - return $this->redirectToRoute(Routes::EDIT_REVISION, ['revisionId' => $revisionId]); + if (!$restoredRevision) { + throw new \RuntimeException(\sprintf('Put back failed for ouuid "%s"', $ouuid)); + } + + return $this->redirectToRoute(Routes::EDIT_REVISION, ['revisionId' => $restoredRevision->getId()]); } private function emptyTrashSelection(ContentType $contentType, string ...$ouuids): Response @@ -103,10 +107,10 @@ private function putBackSelection(ContentType $contentType, string ...$ouuids): throw $this->createAccessDeniedException(); } - $revisionId = $this->dataService->trashPutBack($contentType, ...$ouuids); + $restoredRevision = $this->dataService->trashPutBackAsDraft($contentType, ...$ouuids); - if ($revisionId) { - return $this->redirectToRoute(Routes::EDIT_REVISION, ['revisionId' => $revisionId]); + if ($restoredRevision) { + return $this->redirectToRoute(Routes::EDIT_REVISION, ['revisionId' => $restoredRevision->getId()]); } return $this->redirectToRoute(Routes::DRAFT_IN_PROGRESS, ['contentTypeId' => $contentType->getId()]); diff --git a/EMS/core-bundle/src/Exception/DuplicateOuuidException.php b/EMS/core-bundle/src/Exception/DuplicateOuuidException.php index 5e6eaeddb..319b4f51f 100644 --- a/EMS/core-bundle/src/Exception/DuplicateOuuidException.php +++ b/EMS/core-bundle/src/Exception/DuplicateOuuidException.php @@ -1,7 +1,15 @@ createQueryBuilder('r') - ->select('COUNT(r)'); - $qb->where($qb->expr()->eq('r.ouuid', ':ouuid')); - $qb->andWhere($qb->expr()->eq('r.contentType', ':contentType')); - $qb->setParameter('ouuid', $ouuid); - $qb->setParameter('contentType', $contentType); + $qb = $this->createQueryBuilder('r'); + $qb + ->select('COUNT(r.id)') + ->andWhere($qb->expr()->eq('r.ouuid', ':ouuid')) + ->andWhere($qb->expr()->eq('r.contentType', ':contentType')) + ->setParameters([ + 'ouuid' => $ouuid, + 'contentType' => $contentType, + ]); return (int) $qb->getQuery()->getSingleScalarResult(); } @@ -475,20 +478,19 @@ public function finaliseRevision(ContentType $contentType, string $ouuid, \DateT public function getCurrentRevision(ContentType $contentType, string $ouuid): ?Revision { - $qb = $this->createQueryBuilder('r')->select() - ->where('r.contentType = ?2') - ->andWhere('r.ouuid = ?3') - ->andWhere('r.endTime is null') - ->setParameter(2, $contentType) - ->setParameter(3, $ouuid); + $qb = $this->createQueryBuilder('r'); + $qb + ->andWhere($qb->expr()->eq('r.ouuid', ':ouuid')) + ->andWhere($qb->expr()->eq('r.contentType', ':contentType')) + ->andWhere($qb->expr()->isNull('r.endTime')) + ->setParameters([ + 'ouuid' => $ouuid, + 'contentType' => $contentType, + ]); - /** @var Revision[] $currentRevision */ - $currentRevision = $qb->getQuery()->execute(); - if (isset($currentRevision[0])) { - return $currentRevision[0]; - } else { - return null; - } + $revision = $qb->getQuery()->getOneOrNullResult(); + + return $revision instanceof Revision ? $revision : null; } public function publishRevision(Revision $revision, bool $draft = false): int @@ -833,6 +835,7 @@ public function findTrashRevisions(ContentType $contentType, string ...$ouuids): ->andWhere($qb->expr()->eq('c.id', ':content_type_id')) ->andWhere($qb->expr()->in('r.ouuid', ':ouuids')) ->andWhere($qb->expr()->eq('r.deleted', $qb->expr()->literal(true))) + ->orderBy('r.startTime', 'DESC') ->setParameter('content_type_id', $contentType->getId()) ->setParameter('ouuids', $ouuids, ArrayParameterType::STRING) ; diff --git a/EMS/core-bundle/src/Service/DataService.php b/EMS/core-bundle/src/Service/DataService.php index cd57d7424..137dcc671 100644 --- a/EMS/core-bundle/src/Service/DataService.php +++ b/EMS/core-bundle/src/Service/DataService.php @@ -49,7 +49,6 @@ use Symfony\Component\Form\FormInterface; use Symfony\Component\Form\FormRegistryInterface; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; -use Symfony\Component\HttpKernel\Exception\ConflictHttpException; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; @@ -443,36 +442,14 @@ public function createData(?string $ouuid, array $rawdata, ContentType $contentT $newRevision->setEndTime(null); $newRevision->setDeleted(false); $newRevision->setDraft(true); - if ($byARealUser) { - $token = $this->tokenStorage->getToken(); - if (null === $token) { - throw new \RuntimeException('Unexpected null token'); - } - $newRevision->setLockBy($token->getUserIdentifier()); - } else { - $newRevision->setLockBy('DATA_SERVICE'); - } - $newRevision->setLockUntil($until); - $newRevision->setRawData($rawdata); - $em = $this->doctrine->getManager(); - if (!empty($ouuid)) { - $revisionRepository = $em->getRepository(Revision::class); - $anotherObject = $revisionRepository->findOneBy([ - 'contentType' => $contentType, - 'ouuid' => $newRevision->getOuuid(), - 'endTime' => null, - ]); + $lockBy = $byARealUser ? $this->userService->getCurrentUser()->getUserIdentifier() : 'DATA_SERVICE'; + $newRevision->setLockBy($lockBy); - if (!empty($anotherObject)) { - throw new ConflictHttpException('Duplicate OUUID '.$ouuid.' for this content type'); - } - } - - $em->persist($newRevision); - $em->flush(); + $newRevision->setLockUntil($until); + $newRevision->setRawData($rawdata); - return $newRevision; + return $this->createNewRevision($newRevision); } /** @@ -934,15 +911,9 @@ public function getNewestRevision(string $type, string $ouuid): Revision public function newDocument(ContentType $contentType, ?string $ouuid = null, ?array $rawData = null, ?string $username = null): Revision { $this->hasCreateRights($contentType); - /** @var RevisionRepository $revisionRepository */ - $revisionRepository = $this->em->getRepository(Revision::class); $revision = new Revision(); - if (null !== $ouuid && $revisionRepository->countRevisions($ouuid, $contentType)) { - throw new DuplicateOuuidException(); - } - if (!empty($contentType->getDefaultValue())) { try { $template = $this->twig->createTemplate($contentType->getDefaultValue()); @@ -1026,10 +997,7 @@ public function newDocument(ContentType $contentType, ?string $ouuid = null, ?ar } $this->setMetaFields($revision); - $this->em->persist($revision); - $this->em->flush(); - - return $revision; + return $this->createNewRevision($revision); } /** @@ -1277,25 +1245,23 @@ public function trashEmpty(ContentType $contentType, string ...$ouuids): void $this->em->flush(); } - public function trashPutBack(ContentType $contentType, string ...$ouuids): null|int + public function trashPutBackAsDraft(ContentType $contentType, string ...$ouuids): null|Revision { - $revisionIds = []; $revisions = $this->revRepository->findTrashRevisions($contentType, ...$ouuids); foreach ($revisions as $revision) { - $this->lockRevision($revision); $revision->setDeleted(false); $revision->setDeletedBy(null); if (null === $revision->getEndTime()) { + $this->lockRevision($revision); $revision->setDraft(true); - $revisionIds[] = $revision->getId(); $this->auditLogger->notice('log.revision.restored', LogRevisionContext::update($revision)); } $this->em->persist($revision); } $this->em->flush(); - return 1 === \count($ouuids) ? \array_shift($revisionIds) : null; + return 1 === \count($ouuids) ? \array_shift($revisions) : null; } /** @@ -1985,4 +1951,36 @@ public function getAllDrafts(): array { return $this->revRepository->findAllDrafts(); } + + private function createNewRevision(Revision $revision): Revision + { + $ouuid = $revision->getOuuid(); + $contentType = $revision->giveContentType(); + + $currentRevision = $ouuid ? $this->revRepository->getCurrentRevision($contentType, $ouuid) : null; + if ($ouuid && $currentRevision && !$currentRevision->getDeleted()) { + throw new DuplicateOuuidException($ouuid, $contentType->getName()); + } + + $this->em->getConnection()->beginTransaction(); + + try { + $restoredDraft = $currentRevision ? $this->trashPutBackAsDraft($contentType, $currentRevision->giveOuuid()) : null; + + if ($restoredDraft) { + $restoredDraft->setDraft(false); + $restoredDraft->setEndTime($revision->getStartTime()); + $this->unlockRevision($restoredDraft); + } + + $this->em->persist($revision); + $this->em->flush(); + $this->em->getConnection()->commit(); + } catch (\Throwable $e) { + $this->em->getConnection()->rollBack(); + throw $e; + } + + return $revision; + } } From 9a2ece5f0ce7ffc60990d48bc3093bdf9c4ab1f4 Mon Sep 17 00:00:00 2001 From: David mattei Date: Mon, 15 Jul 2024 10:01:10 +0200 Subject: [PATCH 3/3] refactor(core): CoreControllerTrait instead of abstract class (#956) --- .github/workflows/ci.yml | 6 ++---- .../src/Controller/ContentManagement/ReleaseController.php | 7 +++++-- ...{AbstractCoreController.php => CoreControllerTrait.php} | 3 +-- 3 files changed, 8 insertions(+), 8 deletions(-) rename EMS/core-bundle/src/Controller/{AbstractCoreController.php => CoreControllerTrait.php} (77%) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ee1e89775..29842ed3e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,9 +31,7 @@ jobs: uses: actions/cache/restore@v4 with: path: .cache - key: ci-${{ github.ref_name }}-${{ github.run_id }} - restore-keys: | - ci-${{ github.ref_name }}- + key: ci-${{ github.ref_name }} - name: "Restore Composer Cache" uses: actions/cache/restore@v4 @@ -62,7 +60,7 @@ jobs: if: always() with: path: .cache - key: ci-${{ github.ref_name }}-${{ github.run_id }} + key: ci-${{ github.ref_name }} - name: "Save vendor directory" if: steps.restore-composer.outputs.cache-hit != 'true' diff --git a/EMS/core-bundle/src/Controller/ContentManagement/ReleaseController.php b/EMS/core-bundle/src/Controller/ContentManagement/ReleaseController.php index 7576670df..cb65d7cb6 100644 --- a/EMS/core-bundle/src/Controller/ContentManagement/ReleaseController.php +++ b/EMS/core-bundle/src/Controller/ContentManagement/ReleaseController.php @@ -4,7 +4,7 @@ namespace EMS\CoreBundle\Controller\ContentManagement; -use EMS\CoreBundle\Controller\AbstractCoreController; +use EMS\CoreBundle\Controller\CoreControllerTrait; use EMS\CoreBundle\Core\DataTable\DataTableFactory; use EMS\CoreBundle\Core\Revision\Release\ReleaseRevisionType; use EMS\CoreBundle\DataTable\Type\Release\ReleaseOverviewDataTableType; @@ -20,11 +20,14 @@ use EMS\CoreBundle\Routes; use EMS\CoreBundle\Service\ReleaseService; use Psr\Log\LoggerInterface; +use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; -final class ReleaseController extends AbstractCoreController +final class ReleaseController extends AbstractController { + use CoreControllerTrait; + public function __construct( private readonly LoggerInterface $logger, private readonly ReleaseService $releaseService, diff --git a/EMS/core-bundle/src/Controller/AbstractCoreController.php b/EMS/core-bundle/src/Controller/CoreControllerTrait.php similarity index 77% rename from EMS/core-bundle/src/Controller/AbstractCoreController.php rename to EMS/core-bundle/src/Controller/CoreControllerTrait.php index d9c9a9b86..abc8752cc 100644 --- a/EMS/core-bundle/src/Controller/AbstractCoreController.php +++ b/EMS/core-bundle/src/Controller/CoreControllerTrait.php @@ -4,11 +4,10 @@ namespace EMS\CoreBundle\Controller; -use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\Form\Form; use Symfony\Component\Form\FormInterface; -abstract class AbstractCoreController extends AbstractController +trait CoreControllerTrait { protected function getClickedButtonName(FormInterface $form): ?string {