diff --git a/src/Controller/Administrative/Disposition/DocumentUploadController.php b/src/Controller/Administrative/Disposition/DocumentUploadController.php index a80e238..8400a96 100644 --- a/src/Controller/Administrative/Disposition/DocumentUploadController.php +++ b/src/Controller/Administrative/Disposition/DocumentUploadController.php @@ -13,6 +13,7 @@ use Doctrine\ORM\EntityManagerInterface; use Flagception\Manager\FeatureManagerInterface; use Psr\Log\LoggerInterface; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; +use Symfony\Component\Form\FormError; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Routing\Attribute\Route; @@ -49,6 +50,12 @@ class DocumentUploadController extends AbstractController $form = $this->createFormBuilder()->getForm(); $form->handleRequest($request); + // Checked before processUpload() removes the existing document, which it replaces. + if ($form->isSubmitted() + && true === $this->uploadHandler->getUploadSession()->getUploadsByType($type)->isEmpty()) { + $form->addError(new FormError('Bitte lade zuerst eine Datei hoch')); + } + if ($form->isSubmitted() && $form->isValid()) { $this->processUpload($disposition, $type); @@ -110,15 +117,17 @@ class DocumentUploadController extends AbstractController $disposition->addDocument($upload); - $this->entityManager->flush(); - $this->uploadHandler->moveUploadSessionFilesFromOrphanage($type, $uploadSession); $this->uploadHandler->removeUploadSessionUploadsByType($type); + // Before the flush: converting a non-PDF renames the file, and the new filename has to + // be persisted with the row. if (true === $this->featureManager->isActive('sanitize_uploads')) { $this->uploadHandler->ensurePdf($upload); } + $this->entityManager->flush(); + $this->eventDispatcher->dispatch(new DocumentUploadedEvent($upload), DocumentUploadedEvent::NAME); if (Upload::TYPE_CONTRACT === $type) { $this->eventDispatcher->dispatch(new DocumentConfirmedEvent($upload), DocumentConfirmedEvent::NAME); diff --git a/src/Controller/Teamer/Disposition/DetailController.php b/src/Controller/Teamer/Disposition/DetailController.php index 348c447..65e1025 100644 --- a/src/Controller/Teamer/Disposition/DetailController.php +++ b/src/Controller/Teamer/Disposition/DetailController.php @@ -79,6 +79,14 @@ class DetailController extends AbstractController private function uploadContract(Disposition $disposition, UploadSessionDto $uploadSession): void { + // The session can hold files of other types, so a non-empty session alone does not + // mean there is anything to replace the existing document with. + if (true === $uploadSession->getUploadsByType(Upload::TYPE_CONTRACT)->isEmpty()) { + $this->addFlash('error', 'Bitte lade zuerst den Honorarvertrag hoch'); + + return; + } + if (null !== $existingUpload = $disposition->getDocumentByType(Upload::TYPE_CONTRACT)) { $this->entityManager->remove($existingUpload); } @@ -116,6 +124,14 @@ class DetailController extends AbstractController private function uploadInvoice(Disposition $disposition, UploadSessionDto $uploadSession): void { + // The session can hold files of other types, so a non-empty session alone does not + // mean there is anything to replace the existing document with. + if (true === $uploadSession->getUploadsByType(Upload::TYPE_INVOICE)->isEmpty()) { + $this->addFlash('error', 'Bitte lade zuerst die Honorarnote hoch'); + + return; + } + if (null !== $existingUpload = $disposition->getDocumentByType(Upload::TYPE_INVOICE)) { $this->entityManager->remove($existingUpload); } diff --git a/src/EventListener/DeleteUploadListener.php b/src/EventListener/DeleteUploadListener.php index afc433b..74a8197 100644 --- a/src/EventListener/DeleteUploadListener.php +++ b/src/EventListener/DeleteUploadListener.php @@ -5,31 +5,70 @@ namespace App\EventListener; use App\Entity\Upload; use App\Service\Upload\UploadHandler; use Doctrine\Bundle\DoctrineBundle\Attribute\AsDoctrineListener; -use Doctrine\ORM\Event\PreRemoveEventArgs; +use Doctrine\ORM\Event\OnFlushEventArgs; +use Doctrine\ORM\Event\PostFlushEventArgs; use Doctrine\ORM\Events; use Liip\ImagineBundle\Imagine\Cache\CacheManager; +use Symfony\Contracts\Service\ResetInterface; -#[AsDoctrineListener(event: Events::preRemove)] -class DeleteUploadListener +/** + * Removes the files of deleted uploads from disk, once the deletion has been committed. + * + * Doctrine dispatches preRemove inside EntityManager::remove(), not at flush. Deleting the file + * there meant that a request which removed the old upload and then failed before its flush - + * a replacement that could not be built - lost the file while the row survived. The files are + * therefore collected at onFlush and only deleted at postFlush, which is reached only after the + * transaction has committed. A flush that throws never gets there, and the next onFlush drops + * what it collected. + */ +#[AsDoctrineListener(event: Events::onFlush)] +#[AsDoctrineListener(event: Events::postFlush)] +class DeleteUploadListener implements ResetInterface { + /** + * @var array + */ + private array $pending = []; + public function __construct( private readonly UploadHandler $uploadHandler, private readonly CacheManager $cacheManager, ) { } - public function preRemove(PreRemoveEventArgs $eventArgs): void + public function onFlush(OnFlushEventArgs $args): void { - $object = $eventArgs->getObject(); + $this->pending = []; - if (!$object instanceof Upload) { + // Includes uploads removed by cascade, e.g. the documents of a deleted disposition. + foreach ($args->getObjectManager()->getUnitOfWork()->getScheduledEntityDeletions() as $entity) { + if ($entity instanceof Upload) { + $this->pending[] = $entity; + } + } + } + + public function postFlush(PostFlushEventArgs $args): void + { + if (0 === count($this->pending)) { return; } - // Remove uploaded original - $this->uploadHandler->removeUploadedFile($object); + // Taken and cleared up front, in case anything downstream flushes again. + $pending = $this->pending; + $this->pending = []; - // Remove potential variants from public folders - $this->cacheManager->remove($object->getFilename()); + foreach ($pending as $upload) { + // Remove uploaded original + $this->uploadHandler->removeUploadedFile($upload); + + // Remove potential variants from public folders + $this->cacheManager->remove($upload->getFilename()); + } + } + + public function reset(): void + { + $this->pending = []; } } diff --git a/templates/administrative/disposition/modal_document_upload.html.twig b/templates/administrative/disposition/modal_document_upload.html.twig index e735d14..1c6b819 100644 --- a/templates/administrative/disposition/modal_document_upload.html.twig +++ b/templates/administrative/disposition/modal_document_upload.html.twig @@ -5,6 +5,7 @@ {% block content %} {% set formAttr = stimulus_controller('form-upload-guard')|stimulus_action('form-upload-guard', 'guard', 'submit') %} {{ form_start(form, { 'attr': { 'hx-post': app.request.uri, 'hx-target': '#htmx-modal', 'hx-swap': 'outerHTML' }|merge(formAttr.toArray()) }) }} + {{ form_errors(form) }}
{% include '_partials/_upload_collection_form.html.twig' with { 'endpoint_upload': path(endpointUpload), diff --git a/tests/EventListener/DeleteUploadListenerTest.php b/tests/EventListener/DeleteUploadListenerTest.php new file mode 100644 index 0000000..d7b574e --- /dev/null +++ b/tests/EventListener/DeleteUploadListenerTest.php @@ -0,0 +1,144 @@ + + */ + private array $createdFiles = []; + + protected function setUp(): void + { + self::bootKernel(); + + $this->entityManager = self::getContainer()->get(EntityManagerInterface::class); + $this->uploadHandler = self::getContainer()->get(UploadHandler::class); + + $this->entityManager->createQuery(sprintf('DELETE FROM %s', Upload::class))->execute(); + } + + protected function tearDown(): void + { + (new Filesystem())->remove($this->createdFiles); + + parent::tearDown(); + } + + public function testTheFileIsRemovedOnceTheDeletionIsFlushed(): void + { + $upload = $this->storedUpload(); + + $this->entityManager->remove($upload); + $this->entityManager->flush(); + + $this->assertFileDoesNotExist($this->filepath($upload)); + } + + public function testTheFileIsKeptWhileTheDeletionIsNotFlushed(): void + { + $upload = $this->storedUpload(); + + $this->entityManager->remove($upload); + + $this->assertFileExists($this->filepath($upload)); + } + + public function testTheFileIsKeptWhenTheFlushFails(): void + { + $upload = $this->storedUpload(); + $filepath = $this->filepath($upload); + + $this->entityManager->remove($upload); + // No filename: violates NOT NULL, so the flush rolls back. + $this->entityManager->persist((new Upload()) + ->setType(Upload::TYPE_DOCUMENT) + ->setOriginalFilename('broken.pdf') + ->setMimeType('application/pdf') + ->setSize(1024) + ); + + try { + $this->entityManager->flush(); + $this->fail('The flush was expected to fail'); + } catch (DBALException) { + } + + $this->assertFileExists($filepath); + } + + public function testAFailedFlushDoesNotHandItsFilesToTheNextOne(): void + { + $upload = $this->storedUpload(); + $filepath = $this->filepath($upload); + + $this->entityManager->remove($upload); + $this->entityManager->persist((new Upload()) + ->setType(Upload::TYPE_DOCUMENT) + ->setOriginalFilename('broken.pdf') + ->setMimeType('application/pdf') + ->setSize(1024) + ); + + try { + $this->entityManager->flush(); + } catch (DBALException) { + } + + /** @var EntityManagerInterface $entityManager */ + $entityManager = self::getContainer()->get(ManagerRegistry::class)->resetManager(); + $entityManager->persist($this->upload('unrelated.pdf')); + $entityManager->flush(); + + $this->assertFileExists($filepath); + } + + private function storedUpload(): Upload + { + $upload = $this->upload(sprintf('test-delete-upload-%s.pdf', bin2hex(random_bytes(8)))); + $this->entityManager->persist($upload); + $this->entityManager->flush(); + + $filepath = $this->filepath($upload); + (new Filesystem())->dumpFile($filepath, '%PDF-1.4'); + $this->createdFiles[] = $filepath; + + return $upload; + } + + private function upload(string $filename): Upload + { + return (new Upload()) + ->setType(Upload::TYPE_DOCUMENT) + ->setFilename($filename) + ->setOriginalFilename($filename) + ->setMimeType('application/pdf') + ->setSize(1024) + ; + } + + private function filepath(Upload $upload): string + { + return $this->uploadHandler->getUploadFilepath($upload); + } +}