From 5d51885c8267a90c65d48f0c1512e1ac6f1d8d6e Mon Sep 17 00:00:00 2001
From: Marijn Doeve <marijn@doeve.me>
Date: Sat, 4 Jul 2026 22:07:13 +0200
Subject: [PATCH] fix: address CodeRabbit review findings

- Use FlashType enum in clearQuiz/finalizeQuiz/unfinalizeQuiz (was raw 'success'/'error' strings)
- Catch UniqueConstraintViolationException in addLabel to handle concurrent duplicate inserts
- Wrap assignToQuiz in a transaction with PESSIMISTIC_WRITE lock to serialise concurrent assignments of the same BankQuestion
---
 .../Backoffice/QuestionBankController.php     | 11 +++--
 src/Controller/Backoffice/QuizController.php  |  8 ++--
 src/Service/QuestionBankService.php           | 48 +++++++++++--------
 3 files changed, 39 insertions(+), 28 deletions(-)

diff --git a/src/Controller/Backoffice/QuestionBankController.php b/src/Controller/Backoffice/QuestionBankController.php
index 5729583..494db85 100644
--- a/src/Controller/Backoffice/QuestionBankController.php
+++ b/src/Controller/Backoffice/QuestionBankController.php
@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace Tvdt\Controller\Backoffice;
 
+use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
 use Doctrine\ORM\EntityManagerInterface;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
@@ -215,9 +216,13 @@ class QuestionBankController extends AbstractController
 
         $exists = $season->questionLabels->exists(static fn (int $key, QuestionLabel $label): bool => $label->name === $name);
         if (!$exists) {
-            $season->addQuestionLabel(new QuestionLabel($name));
-            $this->em->flush();
-            $this->addFlash(FlashType::Success, $this->translator->trans('Label added'));
+            try {
+                $season->addQuestionLabel(new QuestionLabel($name));
+                $this->em->flush();
+                $this->addFlash(FlashType::Success, $this->translator->trans('Label added'));
+            } catch (UniqueConstraintViolationException) {
+                // Concurrent request already inserted the same label; treat as a no-op
+            }
         }
 
         return $this->redirectToRoute('tvdt_backoffice_question_bank', ['seasonCode' => $season->seasonCode]);
diff --git a/src/Controller/Backoffice/QuizController.php b/src/Controller/Backoffice/QuizController.php
index 59d8749..a098dcb 100644
--- a/src/Controller/Backoffice/QuizController.php
+++ b/src/Controller/Backoffice/QuizController.php
@@ -293,9 +293,9 @@ class QuizController extends AbstractController
             $this->quizRepository->clearQuiz($quiz);
             $quiz->finalizedAt = null;
             $this->em->flush();
-            $this->addFlash('success', $this->translator->trans('Quiz cleared and no longer finalized'));
+            $this->addFlash(FlashType::Success, $this->translator->trans('Quiz cleared and no longer finalized'));
         } catch (ErrorClearingQuizException) {
-            $this->addFlash('error', $this->translator->trans('Error clearing quiz'));
+            $this->addFlash(FlashType::Danger, $this->translator->trans('Error clearing quiz'));
         }
 
         return $this->redirectToRoute('tvdt_backoffice_quiz', ['seasonCode' => $quiz->season->seasonCode, 'quiz' => $quiz->id]);
@@ -316,7 +316,7 @@ class QuizController extends AbstractController
         } elseif (!$quiz->isFinalized()) {
             $quiz->finalizedAt = new DateTimeImmutable();
             $this->em->flush();
-            $this->addFlash('success', $this->translator->trans('Quiz finalized'));
+            $this->addFlash(FlashType::Success, $this->translator->trans('Quiz finalized'));
         }
 
         return $this->redirectToRoute('tvdt_backoffice_quiz', ['seasonCode' => $quiz->season->seasonCode, 'quiz' => $quiz->id]);
@@ -339,7 +339,7 @@ class QuizController extends AbstractController
         } else {
             $quiz->finalizedAt = null;
             $this->em->flush();
-            $this->addFlash('success', $this->translator->trans('Quiz is no longer finalized'));
+            $this->addFlash(FlashType::Success, $this->translator->trans('Quiz is no longer finalized'));
         }
 
         return $this->redirectToRoute('tvdt_backoffice_quiz', ['seasonCode' => $quiz->season->seasonCode, 'quiz' => $quiz->id]);
diff --git a/src/Service/QuestionBankService.php b/src/Service/QuestionBankService.php
index 175d802..96f86ce 100644
--- a/src/Service/QuestionBankService.php
+++ b/src/Service/QuestionBankService.php
@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace Tvdt\Service;
 
+use Doctrine\DBAL\LockMode;
 use Doctrine\ORM\EntityManagerInterface;
 use Symfony\Component\ObjectMapper\ObjectMapperInterface;
 use Tvdt\Entity\Answer;
@@ -37,34 +38,39 @@ final readonly class QuestionBankService
             throw new QuizLockedException();
         }
 
-        if (!$bankQuestion->canBeAssigned() || $bankQuestion->isUsedInQuiz($quiz)) {
-            throw new BankQuestionAlreadyUsedException();
-        }
+        $this->entityManager->wrapInTransaction(function () use ($bankQuestion, $quiz): void {
+            // Pessimistic write lock serialises concurrent assignment attempts for the same BankQuestion
+            $this->entityManager->lock($bankQuestion, LockMode::PESSIMISTIC_WRITE);
 
-        $maxOrdering = 0;
-        foreach ($quiz->questions as $existingQuestion) {
-            $maxOrdering = max($maxOrdering, $existingQuestion->ordering);
-        }
+            if (!$bankQuestion->canBeAssigned() || $bankQuestion->isUsedInQuiz($quiz)) {
+                throw new BankQuestionAlreadyUsedException();
+            }
 
-        /** @var Question $question */
-        $question = $this->objectMapper->map($bankQuestion, Question::class);
-        $question->ordering = $maxOrdering + 1;
+            $maxOrdering = 0;
+            foreach ($quiz->questions as $existingQuestion) {
+                $maxOrdering = max($maxOrdering, $existingQuestion->ordering);
+            }
 
-        foreach ($bankQuestion->answers as $bankAnswer) {
-            /** @var Answer $answer */
-            $answer = $this->objectMapper->map($bankAnswer, Answer::class);
-            $question->addAnswer($answer);
-        }
+            /** @var Question $question */
+            $question = $this->objectMapper->map($bankQuestion, Question::class);
+            $question->ordering = $maxOrdering + 1;
 
-        $quiz->addQuestion($question);
+            foreach ($bankQuestion->answers as $bankAnswer) {
+                /** @var Answer $answer */
+                $answer = $this->objectMapper->map($bankAnswer, Answer::class);
+                $question->addAnswer($answer);
+            }
 
-        $usage = new BankQuestionUsage($bankQuestion, $quiz);
-        $usage->question = $question;
+            $quiz->addQuestion($question);
 
-        $bankQuestion->addUsage($usage);
+            $usage = new BankQuestionUsage($bankQuestion, $quiz);
+            $usage->question = $question;
 
-        $this->entityManager->persist($question);
-        $this->entityManager->flush();
+            $bankQuestion->addUsage($usage);
+
+            $this->entityManager->persist($question);
+            $this->entityManager->flush();
+        });
     }
 
     /**