From 287c299c41fc99311aa89ee8eff13ce3930bd700 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= Date: Wed, 29 Apr 2026 11:18:52 +0200 Subject: [PATCH] feat: improved performance by avoiding redundant MailJet API calls --- api.http | 4 +- src/Service/NewsletterManager.php | 61 +++++++++---------------- tests/Service/NewsletterManagerTest.php | 51 +++++++++++++-------- 3 files changed, 57 insertions(+), 59 deletions(-) diff --git a/api.http b/api.http index b2c6272..6f28aa8 100644 --- a/api.http +++ b/api.http @@ -116,7 +116,9 @@ Authorization: Bearer {{$auth.token("oauth2_newsletter")}} { "email": "{{$random.email}}", - "listIds": [10321569] + "firstName": "{{$random.address.firstName}}", + "lastName": "{{$random.address.lastName}}", + "listIds": [10321569,10321382,10321383] } ### API pickups planning webhook diff --git a/src/Service/NewsletterManager.php b/src/Service/NewsletterManager.php index 352d68f..6acd7b4 100644 --- a/src/Service/NewsletterManager.php +++ b/src/Service/NewsletterManager.php @@ -96,23 +96,26 @@ class NewsletterManager } } - $subscribedListIds = $this->subscribedMailjetListIds($normalizedEmail, $normalizedListIds); - $missingListIds = array_values(array_diff($normalizedListIds, $subscribedListIds)); $hasConfirmedOptIn = null !== $this->consentRepository->findActiveByEmail($normalizedEmail); - if ([] === $missingListIds) { - $this->recordSubscription($normalizedEmail, $normalizedListIds, [], $normalizedFirstName, $normalizedLastName); + // We trust our local consent state for DOI decisions. If Mailjet is ahead of the database, + // we accept a redundant confirmation cycle and reconcile the contact on confirmation. + if (true === $hasConfirmedOptIn) { + $confirmedListIds = $this->confirmedMailjetListIds($normalizedEmail, $normalizedListIds); + $missingListIds = array_values(array_diff($normalizedListIds, $confirmedListIds)); - return new NewsletterSubscriptionRequestResult( - $normalizedEmail, - $normalizedListIds, - NewsletterSubscriptionRequestResult::STATE_SUBSCRIBED, - false, - $this->createListStates($normalizedListIds, NewsletterSubscriptionRequestResult::LIST_STATE_ALREADY_REGISTERED), - ); - } + if ([] === $missingListIds) { + $this->recordSubscription($normalizedEmail, $normalizedListIds, [], $normalizedFirstName, $normalizedLastName); + + return new NewsletterSubscriptionRequestResult( + $normalizedEmail, + $normalizedListIds, + NewsletterSubscriptionRequestResult::STATE_SUBSCRIBED, + false, + $this->createListStates($normalizedListIds, NewsletterSubscriptionRequestResult::LIST_STATE_ALREADY_REGISTERED), + ); + } - if (true === $hasConfirmedOptIn || [] !== $subscribedListIds || true === $this->isSubscribedToKnownList($normalizedEmail, $knownNormalizedListIds, $normalizedListIds)) { $this->recordSubscription($normalizedEmail, $normalizedListIds, $missingListIds, $normalizedFirstName, $normalizedLastName); return new NewsletterSubscriptionRequestResult( @@ -389,17 +392,18 @@ class NewsletterManager * * @return list */ - private function subscribedMailjetListIds(string $email, array $mailjetListIds): array + private function confirmedMailjetListIds(string $email, array $mailjetListIds): array { - $subscribedListIds = []; + $confirmedListIds = []; foreach ($mailjetListIds as $mailjetListId) { - if (true === $this->newsletterService->isSubscribed($email, $mailjetListId)) { - $subscribedListIds[] = $mailjetListId; + $consent = $this->consentRepository->findOneByEmailAndListId($email, $mailjetListId); + if (null !== $consent && true === $consent->isConfirmed()) { + $confirmedListIds[] = $mailjetListId; } } - return $subscribedListIds; + return $confirmedListIds; } /** @@ -484,27 +488,6 @@ class NewsletterManager } } - /** - * @param list $knownMailjetListIds - * @param list $alreadyCheckedListIds - */ - private function isSubscribedToKnownList(string $normalizedEmail, array $knownMailjetListIds, array $alreadyCheckedListIds): bool - { - $alreadyCheckedListIdMap = array_fill_keys($alreadyCheckedListIds, true); - - foreach ($knownMailjetListIds as $mailjetListId) { - if (true === isset($alreadyCheckedListIdMap[$mailjetListId])) { - continue; - } - - if (true === $this->newsletterService->isSubscribed($normalizedEmail, $mailjetListId)) { - return true; - } - } - - return false; - } - private function defaultMailjetListId(): int { if (null === $this->defaultMailjetListId || '' === trim($this->defaultMailjetListId)) { diff --git a/tests/Service/NewsletterManagerTest.php b/tests/Service/NewsletterManagerTest.php index bc56985..7fbd513 100644 --- a/tests/Service/NewsletterManagerTest.php +++ b/tests/Service/NewsletterManagerTest.php @@ -29,12 +29,10 @@ class NewsletterManagerTest extends TestCase $mailjet = $this->createMock(MailjetApiClient::class); $mailer = $this->createMock(Mailer::class); - $mailjet - ->expects(self::exactly(2)) - ->method('isSubscribed') - ->withConsecutive(['customer@example.com', 1], ['customer@example.com', 2]) - ->willReturn(false); + $mailjet->expects(self::never())->method('isSubscribed'); + $mailjet->expects(self::never())->method('ensureSubscribed'); $consents->expects(self::once())->method('findActiveByEmail')->with('customer@example.com')->willReturn(null); + $consents->expects(self::never())->method('findOneByEmailAndListId'); $repository->expects(self::once())->method('deleteExpiredPendingByEmail')->with('customer@example.com')->willReturn(0); $repository->expects(self::once())->method('findPendingByEmail')->with('customer@example.com')->willReturn(null); @@ -88,8 +86,10 @@ class NewsletterManagerTest extends TestCase $mailjet = $this->createMock(MailjetApiClient::class); $mailer = $this->createMock(Mailer::class); - $mailjet->method('isSubscribed')->willReturn(false); + $mailjet->expects(self::never())->method('isSubscribed'); + $mailjet->expects(self::never())->method('ensureSubscribed'); $consents->expects(self::once())->method('findActiveByEmail')->with('customer@example.com')->willReturn(null); + $consents->expects(self::never())->method('findOneByEmailAndListId'); $repository->method('deleteExpiredPendingByEmail')->willReturn(0); $repository->expects(self::once())->method('findPendingByEmail')->with('customer@example.com')->willReturn($pending); @@ -117,8 +117,10 @@ class NewsletterManagerTest extends TestCase $mailjet = $this->createMock(MailjetApiClient::class); $mailer = $this->createMock(Mailer::class); - $mailjet->method('isSubscribed')->willReturn(false); + $mailjet->expects(self::never())->method('isSubscribed'); + $mailjet->expects(self::never())->method('ensureSubscribed'); $consents->expects(self::once())->method('findActiveByEmail')->with('customer@example.com')->willReturn(null); + $consents->expects(self::never())->method('findOneByEmailAndListId'); $repository->method('deleteExpiredPendingByEmail')->willReturn(0); $repository->expects(self::exactly(2))->method('findPendingByEmail')->with('customer@example.com')->willReturn($pending); @@ -136,20 +138,33 @@ class NewsletterManagerTest extends TestCase self::assertSame('Person', $pending->getLastName()); } - public function testApiRequestRecordsConsentWhenAllListsAreAlreadySubscribed(): void + public function testApiRequestUsesLocalConsentStateForAlreadyRegisteredLists(): void { + $existingConsent = new NewsletterConsent('customer@example.com', 1, 'Old', 'Name'); + $existingConsent->markConfirmed(); + $existingOtherConsent = new NewsletterConsent('customer@example.com', 2, 'Old', 'Name'); + $existingOtherConsent->markConfirmed(); $repository = $this->createMock(NewsletterOptInRequestRepository::class); $consents = $this->createMock(NewsletterConsentRepository::class); $entityManager = $this->createMock(EntityManagerInterface::class); $mailjet = $this->createMock(MailjetApiClient::class); $mailer = $this->createMock(Mailer::class); - $mailjet->method('isSubscribed')->willReturn(true); + $mailjet->expects(self::never())->method('isSubscribed'); + $mailjet->expects(self::never())->method('ensureSubscribed'); $mailjet->expects(self::once())->method('upsertContact')->with('customer@example.com', 'Mia', 'Muster'); - $consents->method('findActiveByEmail')->with('customer@example.com')->willReturn(null); - $consents->method('findOneByEmailAndListId')->willReturn(null); + $consents->method('findActiveByEmail')->with('customer@example.com')->willReturn($existingConsent); + $consents + ->method('findOneByEmailAndListId') + ->willReturnCallback(static function (string $email, int $mailjetListId) use ($existingConsent, $existingOtherConsent): ?NewsletterConsent { + return match ($mailjetListId) { + 1 => $existingConsent, + 2 => $existingOtherConsent, + default => null, + }; + }); $repository->expects(self::once())->method('deletePendingByEmail')->with('customer@example.com')->willReturn(0); - $entityManager->expects(self::exactly(2))->method('persist')->with(self::isInstanceOf(NewsletterConsent::class)); + $entityManager->expects(self::never())->method('persist'); $entityManager->expects(self::once())->method('flush'); $mailer->expects(self::never())->method('createAndSendEmail'); @@ -192,20 +207,18 @@ class NewsletterManagerTest extends TestCase public function testApiRequestDirectlySubscribesMissingListWhenEmailIsSubscribedToKnownList(): void { + $existingConsent = new NewsletterConsent('customer@example.com', 1, 'Old', 'Name'); + $existingConsent->markConfirmed(); $repository = $this->createMock(NewsletterOptInRequestRepository::class); $consents = $this->createMock(NewsletterConsentRepository::class); $entityManager = $this->createMock(EntityManagerInterface::class); $mailjet = $this->createMock(MailjetApiClient::class); $mailer = $this->createMock(Mailer::class); - $mailjet - ->expects(self::exactly(2)) - ->method('isSubscribed') - ->withConsecutive(['customer@example.com', 2], ['customer@example.com', 1]) - ->willReturnOnConsecutiveCalls(false, true); + $mailjet->expects(self::never())->method('isSubscribed'); $mailjet->expects(self::once())->method('upsertContact')->with('customer@example.com', 'Mia', 'Muster'); $mailjet->expects(self::once())->method('ensureSubscribed')->with('customer@example.com', 2); - $consents->method('findActiveByEmail')->with('customer@example.com')->willReturn(null); + $consents->method('findActiveByEmail')->with('customer@example.com')->willReturn($existingConsent); $consents->method('findOneByEmailAndListId')->willReturn(null); $repository->expects(self::once())->method('deletePendingByEmail')->with('customer@example.com')->willReturn(0); $repository->expects(self::never())->method('findPendingByEmail'); @@ -233,7 +246,7 @@ class NewsletterManagerTest extends TestCase $mailjet = $this->createMock(MailjetApiClient::class); $mailer = $this->createMock(Mailer::class); - $mailjet->method('isSubscribed')->with('customer@example.com', 2)->willReturn(false); + $mailjet->expects(self::never())->method('isSubscribed'); $mailjet->expects(self::once())->method('upsertContact')->with('customer@example.com', 'Mia', 'Muster'); $mailjet->expects(self::once())->method('ensureSubscribed')->with('customer@example.com', 2); $consents->method('findActiveByEmail')->with('customer@example.com')->willReturn($existingConsent);