From 6c4ea07e6e1a1e729d9142918986ea8184a380c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= Date: Tue, 14 Jul 2026 09:47:22 +0200 Subject: [PATCH] fix: prevent race condition on newsletter consent confirmation --- .../NewsletterConsentRepository.php | 27 +++++++++++++++++++ src/Service/NewsletterManager.php | 7 ++--- tests/Service/NewsletterManagerTest.php | 9 ++++++- 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/src/Repository/NewsletterConsentRepository.php b/src/Repository/NewsletterConsentRepository.php index c9072e8..eb8aa82 100644 --- a/src/Repository/NewsletterConsentRepository.php +++ b/src/Repository/NewsletterConsentRepository.php @@ -38,4 +38,31 @@ class NewsletterConsentRepository extends ServiceEntityRepository 'mailjetListId' => $mailjetListId, ]); } + + /** + * @param list $mailjetListIds + * + * @return array existing consents indexed by mailjetListId + */ + public function findByEmailAndListIds(string $email, array $mailjetListIds): array + { + if ([] === $mailjetListIds) { + return []; + } + + $consents = $this->createQueryBuilder('c') + ->where('c.email = :email') + ->andWhere('c.mailjetListId IN (:listIds)') + ->setParameter('email', mb_strtolower(trim($email))) + ->setParameter('listIds', $mailjetListIds) + ->getQuery() + ->getResult(); + + $byListId = []; + foreach ($consents as $consent) { + $byListId[$consent->getMailjetListId()] = $consent; + } + + return $byListId; + } } diff --git a/src/Service/NewsletterManager.php b/src/Service/NewsletterManager.php index 5387ca8..9513a3f 100644 --- a/src/Service/NewsletterManager.php +++ b/src/Service/NewsletterManager.php @@ -327,8 +327,7 @@ class NewsletterManager $this->newsletterService->ensureSubscribed($confirmation->getEmail(), $mailjetListId); } - $confirmation->markConfirmed(); - $this->upsertConfirmedConsents($confirmation->getEmail(), $mailjetListIds, $confirmation->getFirstName(), $confirmation->getLastName(), false, false); + $this->upsertConfirmedConsents($confirmation->getEmail(), $mailjetListIds, $confirmation->getFirstName(), $confirmation->getLastName(), flush: false, deletePending: false); $this->entityManager->remove($confirmation); $this->entityManager->flush(); @@ -468,8 +467,10 @@ class NewsletterManager */ private function upsertConfirmedConsents(string $email, array $mailjetListIds, ?string $firstName, ?string $lastName, bool $flush = true, bool $deletePending = true): void { + $existingByListId = $this->consentRepository->findByEmailAndListIds($email, $mailjetListIds); + foreach ($mailjetListIds as $mailjetListId) { - $consent = $this->consentRepository->findOneByEmailAndListId($email, $mailjetListId); + $consent = $existingByListId[$mailjetListId] ?? null; if (null === $consent) { $consent = new NewsletterConsent($email, $mailjetListId, $firstName, $lastName); $this->entityManager->persist($consent); diff --git a/tests/Service/NewsletterManagerTest.php b/tests/Service/NewsletterManagerTest.php index 6e64719..40f9c38 100644 --- a/tests/Service/NewsletterManagerTest.php +++ b/tests/Service/NewsletterManagerTest.php @@ -166,6 +166,10 @@ class NewsletterManagerTest extends TestCase default => null, }; }); + $consents + ->method('findByEmailAndListIds') + ->with('customer@example.com', [1, 2]) + ->willReturn([1 => $existingConsent, 2 => $existingOtherConsent]); $repository->expects(self::once())->method('deletePendingByEmail')->with('customer@example.com')->willReturn(0); $entityManager->expects(self::never())->method('persist'); $entityManager->expects(self::once())->method('flush'); @@ -223,6 +227,7 @@ class NewsletterManagerTest extends TestCase $mailjet->expects(self::once())->method('ensureSubscribed')->with('customer@example.com', 2); $consents->method('findActiveByEmail')->with('customer@example.com')->willReturn($existingConsent); $consents->method('findOneByEmailAndListId')->willReturn(null); + $consents->method('findByEmailAndListIds')->willReturn([]); $repository->expects(self::once())->method('deletePendingByEmail')->with('customer@example.com')->willReturn(0); $repository->expects(self::never())->method('findPendingByEmail'); $entityManager->expects(self::once())->method('persist')->with(self::isInstanceOf(NewsletterConsent::class)); @@ -254,6 +259,7 @@ class NewsletterManagerTest extends TestCase $mailjet->expects(self::once())->method('ensureSubscribed')->with('customer@example.com', 2); $consents->method('findActiveByEmail')->with('customer@example.com')->willReturn($existingConsent); $consents->method('findOneByEmailAndListId')->with('customer@example.com', 2)->willReturn(null); + $consents->method('findByEmailAndListIds')->willReturn([]); $repository->expects(self::once())->method('deletePendingByEmail')->with('customer@example.com')->willReturn(0); $entityManager->expects(self::once())->method('persist')->with(self::isInstanceOf(NewsletterConsent::class)); $entityManager->expects(self::once())->method('flush'); @@ -338,7 +344,7 @@ class NewsletterManagerTest extends TestCase $repository->expects(self::once())->method('findByTokenHash')->with(hash('sha256', $token))->willReturn($confirmation); $repository->expects(self::never())->method('deletePendingByEmail'); - $consents->expects(self::once())->method('findOneByEmailAndListId')->with('customer@example.com', 10321569)->willReturn(null); + $consents->expects(self::once())->method('findByEmailAndListIds')->with('customer@example.com', [10321569])->willReturn([]); $mailjet->expects(self::once())->method('upsertContact')->with('customer@example.com', 'Mia', 'Muster'); $mailjet->expects(self::once())->method('ensureSubscribed')->with('customer@example.com', 10321569); $entityManager->expects(self::once())->method('persist')->with(self::isInstanceOf(NewsletterConsent::class)); @@ -370,6 +376,7 @@ class NewsletterManagerTest extends TestCase $repository->expects(self::once())->method('findByTokenHash')->with(hash('sha256', $token))->willReturn($confirmation); $repository->expects(self::never())->method('deletePendingByEmail'); $consents->method('findOneByEmailAndListId')->willReturn(null); + $consents->method('findByEmailAndListIds')->with('customer@example.com', [1, 2])->willReturn([]); $mailjet ->expects(self::exactly(2)) ->method('ensureSubscribed')