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
This commit is contained in:
2026-07-04 22:07:13 +02:00
parent 8304d8680b
commit 5d51885c82
3 changed files with 39 additions and 28 deletions
@@ -4,6 +4,7 @@ declare(strict_types=1);
namespace Tvdt\Controller\Backoffice; namespace Tvdt\Controller\Backoffice;
use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\EntityManagerInterface;
use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\RedirectResponse;
use Symfony\Component\HttpFoundation\Request; 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); $exists = $season->questionLabels->exists(static fn (int $key, QuestionLabel $label): bool => $label->name === $name);
if (!$exists) { if (!$exists) {
$season->addQuestionLabel(new QuestionLabel($name)); try {
$this->em->flush(); $season->addQuestionLabel(new QuestionLabel($name));
$this->addFlash(FlashType::Success, $this->translator->trans('Label added')); $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]); return $this->redirectToRoute('tvdt_backoffice_question_bank', ['seasonCode' => $season->seasonCode]);
+4 -4
View File
@@ -293,9 +293,9 @@ class QuizController extends AbstractController
$this->quizRepository->clearQuiz($quiz); $this->quizRepository->clearQuiz($quiz);
$quiz->finalizedAt = null; $quiz->finalizedAt = null;
$this->em->flush(); $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) { } 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]); return $this->redirectToRoute('tvdt_backoffice_quiz', ['seasonCode' => $quiz->season->seasonCode, 'quiz' => $quiz->id]);
@@ -316,7 +316,7 @@ class QuizController extends AbstractController
} elseif (!$quiz->isFinalized()) { } elseif (!$quiz->isFinalized()) {
$quiz->finalizedAt = new DateTimeImmutable(); $quiz->finalizedAt = new DateTimeImmutable();
$this->em->flush(); $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]); return $this->redirectToRoute('tvdt_backoffice_quiz', ['seasonCode' => $quiz->season->seasonCode, 'quiz' => $quiz->id]);
@@ -339,7 +339,7 @@ class QuizController extends AbstractController
} else { } else {
$quiz->finalizedAt = null; $quiz->finalizedAt = null;
$this->em->flush(); $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]); return $this->redirectToRoute('tvdt_backoffice_quiz', ['seasonCode' => $quiz->season->seasonCode, 'quiz' => $quiz->id]);
+27 -21
View File
@@ -4,6 +4,7 @@ declare(strict_types=1);
namespace Tvdt\Service; namespace Tvdt\Service;
use Doctrine\DBAL\LockMode;
use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\EntityManagerInterface;
use Symfony\Component\ObjectMapper\ObjectMapperInterface; use Symfony\Component\ObjectMapper\ObjectMapperInterface;
use Tvdt\Entity\Answer; use Tvdt\Entity\Answer;
@@ -37,34 +38,39 @@ final readonly class QuestionBankService
throw new QuizLockedException(); throw new QuizLockedException();
} }
if (!$bankQuestion->canBeAssigned() || $bankQuestion->isUsedInQuiz($quiz)) { $this->entityManager->wrapInTransaction(function () use ($bankQuestion, $quiz): void {
throw new BankQuestionAlreadyUsedException(); // Pessimistic write lock serialises concurrent assignment attempts for the same BankQuestion
} $this->entityManager->lock($bankQuestion, LockMode::PESSIMISTIC_WRITE);
$maxOrdering = 0; if (!$bankQuestion->canBeAssigned() || $bankQuestion->isUsedInQuiz($quiz)) {
foreach ($quiz->questions as $existingQuestion) { throw new BankQuestionAlreadyUsedException();
$maxOrdering = max($maxOrdering, $existingQuestion->ordering); }
}
/** @var Question $question */ $maxOrdering = 0;
$question = $this->objectMapper->map($bankQuestion, Question::class); foreach ($quiz->questions as $existingQuestion) {
$question->ordering = $maxOrdering + 1; $maxOrdering = max($maxOrdering, $existingQuestion->ordering);
}
foreach ($bankQuestion->answers as $bankAnswer) { /** @var Question $question */
/** @var Answer $answer */ $question = $this->objectMapper->map($bankQuestion, Question::class);
$answer = $this->objectMapper->map($bankAnswer, Answer::class); $question->ordering = $maxOrdering + 1;
$question->addAnswer($answer);
}
$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); $quiz->addQuestion($question);
$usage->question = $question;
$bankQuestion->addUsage($usage); $usage = new BankQuestionUsage($bankQuestion, $quiz);
$usage->question = $question;
$this->entityManager->persist($question); $bankQuestion->addUsage($usage);
$this->entityManager->flush();
$this->entityManager->persist($question);
$this->entityManager->flush();
});
} }
/** /**