From a47a69bdd56cb0e93e21f10f47d35cbdad38ebd1 Mon Sep 17 00:00:00 2001 From: David mattei Date: Fri, 12 Jul 2024 13:24:30 +0200 Subject: [PATCH 1/8] refactor: clean countRevisions method --- .../src/Repository/RevisionRepository.php | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/EMS/core-bundle/src/Repository/RevisionRepository.php b/EMS/core-bundle/src/Repository/RevisionRepository.php index 1e2da3530..7a8179b33 100644 --- a/EMS/core-bundle/src/Repository/RevisionRepository.php +++ b/EMS/core-bundle/src/Repository/RevisionRepository.php @@ -349,12 +349,15 @@ public function countByContentType(ContentType $contentType): int public function countRevisions(string $ouuid, ContentType $contentType): int { - $qb = $this->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(); } From d45ee33fbe4c931975a5ea8aab2fe1085d76b5a1 Mon Sep 17 00:00:00 2001 From: David mattei Date: Fri, 12 Jul 2024 14:34:48 +0200 Subject: [PATCH 2/8] refactor: crud controller create clean handling exceptions --- .../ContentManagement/CrudController.php | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 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(), ]); } From bf6511de4463ebe5c691110e1a1613aa0ea1f09b Mon Sep 17 00:00:00 2001 From: David mattei Date: Fri, 12 Jul 2024 14:36:01 +0200 Subject: [PATCH 3/8] refactor: improve getCurrentRevision repo function --- .../src/Repository/RevisionRepository.php | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/EMS/core-bundle/src/Repository/RevisionRepository.php b/EMS/core-bundle/src/Repository/RevisionRepository.php index 7a8179b33..c2338d6e9 100644 --- a/EMS/core-bundle/src/Repository/RevisionRepository.php +++ b/EMS/core-bundle/src/Repository/RevisionRepository.php @@ -478,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()->getSingleResult(); + + return $revision instanceof Revision ? $revision : null; } public function publishRevision(Revision $revision, bool $draft = false): int From 821f972b8003a1f11905f2dc7730a38fe9be49ca Mon Sep 17 00:00:00 2001 From: David mattei Date: Fri, 12 Jul 2024 14:37:25 +0200 Subject: [PATCH 4/8] fix: throw better DuplicateOuuidException - newDocument is called on api update - createData is called from ui --- .../src/Exception/DuplicateOuuidException.php | 8 +++++ EMS/core-bundle/src/Service/DataService.php | 33 +++++-------------- 2 files changed, 17 insertions(+), 24 deletions(-) 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 @@ 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'); - } + + $lockBy = $byARealUser ? $this->userService->getCurrentUser()->getUserIdentifier() : 'DATA_SERVICE'; + $newRevision->setLockBy($lockBy); + $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, - ]); - - if (!empty($anotherObject)) { - throw new ConflictHttpException('Duplicate OUUID '.$ouuid.' for this content type'); - } + if (null !== $ouuid && null !== $this->revRepository->getCurrentRevision($contentType, $ouuid)) { + throw new DuplicateOuuidException($ouuid, $contentType->getName()); } + $em = $this->doctrine->getManager(); $em->persist($newRevision); $em->flush(); @@ -939,8 +924,8 @@ public function newDocument(ContentType $contentType, ?string $ouuid = null, ?ar $revision = new Revision(); - if (null !== $ouuid && $revisionRepository->countRevisions($ouuid, $contentType)) { - throw new DuplicateOuuidException(); + if (null !== $ouuid && $revisionRepository->getCurrentRevision($contentType, $ouuid)) { + throw new DuplicateOuuidException($ouuid, $contentType->getName()); } if (!empty($contentType->getDefaultValue())) { From be06bfe72cae3fa6fcc1b1ddda3142bfed3b6f6b Mon Sep 17 00:00:00 2001 From: David mattei Date: Fri, 12 Jul 2024 19:18:34 +0200 Subject: [PATCH 5/8] fix: dataService trashPutBack return revision Also it only makes sense to lock the draft revision. The other revisions will also be put back, but locking is not needed. --- .../src/Controller/Revision/TrashController.php | 14 +++++++++----- EMS/core-bundle/src/Service/DataService.php | 8 +++----- 2 files changed, 12 insertions(+), 10 deletions(-) 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/Service/DataService.php b/EMS/core-bundle/src/Service/DataService.php index cd1086dd0..2ce36d24a 100644 --- a/EMS/core-bundle/src/Service/DataService.php +++ b/EMS/core-bundle/src/Service/DataService.php @@ -1262,25 +1262,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; } /** From 52a18ed0fd48183a5a846f141ffbc877984c7849 Mon Sep 17 00:00:00 2001 From: David mattei Date: Fri, 12 Jul 2024 19:20:09 +0200 Subject: [PATCH 6/8] fix: added new private method createNewRevision Will check the currentRevision and restore if needed. The restored draft will be closed (endTime) and draft discard. --- EMS/core-bundle/src/Service/DataService.php | 53 +++++++++++++-------- 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/EMS/core-bundle/src/Service/DataService.php b/EMS/core-bundle/src/Service/DataService.php index 2ce36d24a..137dcc671 100644 --- a/EMS/core-bundle/src/Service/DataService.php +++ b/EMS/core-bundle/src/Service/DataService.php @@ -449,15 +449,7 @@ public function createData(?string $ouuid, array $rawdata, ContentType $contentT $newRevision->setLockUntil($until); $newRevision->setRawData($rawdata); - if (null !== $ouuid && null !== $this->revRepository->getCurrentRevision($contentType, $ouuid)) { - throw new DuplicateOuuidException($ouuid, $contentType->getName()); - } - - $em = $this->doctrine->getManager(); - $em->persist($newRevision); - $em->flush(); - - return $newRevision; + return $this->createNewRevision($newRevision); } /** @@ -919,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->getCurrentRevision($contentType, $ouuid)) { - throw new DuplicateOuuidException($ouuid, $contentType->getName()); - } - if (!empty($contentType->getDefaultValue())) { try { $template = $this->twig->createTemplate($contentType->getDefaultValue()); @@ -1011,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); } /** @@ -1968,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 1fe600e8928e7da21ca2f64f2f35727fc233a19e Mon Sep 17 00:00:00 2001 From: David mattei Date: Fri, 12 Jul 2024 19:20:39 +0200 Subject: [PATCH 7/8] fix: getCurrentRevision work with singleResult --- EMS/core-bundle/src/Repository/RevisionRepository.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/EMS/core-bundle/src/Repository/RevisionRepository.php b/EMS/core-bundle/src/Repository/RevisionRepository.php index c2338d6e9..83dc3ca41 100644 --- a/EMS/core-bundle/src/Repository/RevisionRepository.php +++ b/EMS/core-bundle/src/Repository/RevisionRepository.php @@ -488,7 +488,7 @@ public function getCurrentRevision(ContentType $contentType, string $ouuid): ?Re 'contentType' => $contentType, ]); - $revision = $qb->getQuery()->getSingleResult(); + $revision = $qb->getQuery()->getOneOrNullResult(); return $revision instanceof Revision ? $revision : null; } From f5cfe6eef384c86cb78cb2401cbf0bd9b98c3b47 Mon Sep 17 00:00:00 2001 From: David mattei Date: Fri, 12 Jul 2024 19:21:35 +0200 Subject: [PATCH 8/8] fix: findTrashRevisions current needs to be first. Because if we work with a single ouuid, the putBack method will return the current revision as a new draft. --- EMS/core-bundle/src/Repository/RevisionRepository.php | 1 + 1 file changed, 1 insertion(+) diff --git a/EMS/core-bundle/src/Repository/RevisionRepository.php b/EMS/core-bundle/src/Repository/RevisionRepository.php index 83dc3ca41..9b874d77b 100644 --- a/EMS/core-bundle/src/Repository/RevisionRepository.php +++ b/EMS/core-bundle/src/Repository/RevisionRepository.php @@ -835,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) ;