diff --git a/config/parameters.yml.dist b/config/parameters.yml.dist index 1e195cad..bdeba7c6 100644 --- a/config/parameters.yml.dist +++ b/config/parameters.yml.dist @@ -37,6 +37,8 @@ parameters: env(REST_API_BASE_URL): 'http://api.phplist.local/api/v2' app.frontend_base_url: '%%env(FRONT_END_BASE_URL)%%' env(FRONT_END_BASE_URL): 'http://frontend.phplist.local' + parallel_use_with_phplist3: '%%env(parallel_use_with_phplist3)%%' + env(parallel_use_with_phplist3): '0' # Email configuration app.mailer_from: '%%env(MAILER_FROM)%%' diff --git a/config/services/services.yml b/config/services/services.yml index 53a5f84d..f8bf5b97 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -43,6 +43,14 @@ services: autoconfigure: true public: true + PhpList\Core\Domain\Subscription\Service\SubscribePageConfigMigrationService: + autowire: true + autoconfigure: true + + PhpList\Core\Domain\Subscription\Service\Manager\SubscribePageManager: + autowire: true + autoconfigure: true + PhpList\Core\Domain\Messaging\Service\MessageProcessingPreparator: autowire: true autoconfigure: true diff --git a/src/Domain/Configuration/Service/Provider/ConfigProvider.php b/src/Domain/Configuration/Service/Provider/ConfigProvider.php index 0124af9c..3b22285f 100644 --- a/src/Domain/Configuration/Service/Provider/ConfigProvider.php +++ b/src/Domain/Configuration/Service/Provider/ConfigProvider.php @@ -8,6 +8,7 @@ use PhpList\Core\Domain\Configuration\Model\ConfigOption; use PhpList\Core\Domain\Configuration\Repository\ConfigRepository; use Psr\SimpleCache\CacheInterface; +use ValueError; class ConfigProvider { @@ -71,16 +72,32 @@ public function getValue(ConfigOption $key): ?string return $this->defaultConfigs->has($key) ? (string) $this->defaultConfigs->get($key)['value'] : null; } - public function getValueWithNamespace(ConfigOption $key): ?string + public function getValueWithNamespace(ConfigOption|string $key): ?string { - $full = $this->getValue($key); + if ($key instanceof ConfigOption) { + $full = $this->getValue($key); + $keyValue = $key->value; + } else { + $keyValue = $key; + $cacheKey = 'cfg:' . $keyValue; + $full = $this->cache->get($cacheKey); + if ($full === null) { + $full = $this->configRepository->findValueByItem($keyValue); + $this->cache->set($cacheKey, $full, $this->ttlSeconds); + } + } + if ($full !== null && $full !== '') { return $full; } - if (str_contains($key->value, ':')) { - [$parent] = explode(':', $key->value, 2); - $parentKey = ConfigOption::from($parent); + if (str_contains($keyValue, ':')) { + [$parent] = explode(':', $keyValue, 2); + try { + $parentKey = ConfigOption::from($parent); + } catch (ValueError) { + return null; + } return $this->getValue($parentKey); } diff --git a/src/Domain/Subscription/Model/SubscribePage.php b/src/Domain/Subscription/Model/SubscribePage.php index 979b3c4c..3b484920 100644 --- a/src/Domain/Subscription/Model/SubscribePage.php +++ b/src/Domain/Subscription/Model/SubscribePage.php @@ -30,6 +30,9 @@ class SubscribePage implements DomainModel, Identity, OwnableInterface #[ORM\JoinColumn(name: 'owner', referencedColumnName: 'id', nullable: true)] private ?Administrator $owner = null; + /** @var SubscribePageData[] */ + private array $data = []; + public function getId(): ?int { return $this->id; @@ -50,6 +53,12 @@ public function getOwner(): ?Administrator return $this->owner; } + /** @return SubscribePageData[] */ + public function getData(): array + { + return $this->data; + } + public function setTitle(string $title): self { $this->title = $title; @@ -67,4 +76,11 @@ public function setOwner(?Administrator $owner): self $this->owner = $owner; return $this; } + + /** @param SubscribePageData[] $data */ + public function setData(array $data): self + { + $this->data = $data; + return $this; + } } diff --git a/src/Domain/Subscription/Repository/SubscriberPageRepository.php b/src/Domain/Subscription/Repository/SubscriberPageRepository.php index 136b589c..c9f39449 100644 --- a/src/Domain/Subscription/Repository/SubscriberPageRepository.php +++ b/src/Domain/Subscription/Repository/SubscriberPageRepository.php @@ -4,6 +4,8 @@ namespace PhpList\Core\Domain\Subscription\Repository; +use PhpList\Core\Domain\Common\Model\Filter\FilterRequestInterface; +use PhpList\Core\Domain\Common\Model\PaginatedResult; use PhpList\Core\Domain\Common\Repository\AbstractRepository; use PhpList\Core\Domain\Common\Repository\CursorPaginationTrait; use PhpList\Core\Domain\Common\Repository\Interfaces\PaginatableRepositoryInterface; @@ -14,17 +16,92 @@ class SubscriberPageRepository extends AbstractRepository implements Paginatable { use CursorPaginationTrait; - /** @return array{page: SubscribePage, data: SubscribePageData}[] */ - public function findPagesWithData(int $pageId): array + public function findPageWithData(int $pageId): ?SubscribePage { - return $this->createQueryBuilder('p') - ->select('p AS page, d AS data') - ->from(SubscribePage::class, 'p') - ->from(SubscribePageData::class, 'd') + $result = $this->createQueryBuilder('p') + ->select('p AS page', 'd AS data') + ->leftJoin( + SubscribePageData::class, + 'd', + 'ON', + 'd.id = p.id' + ) ->where('p.id = :id') - ->andWhere('d.id = p.id') ->setParameter('id', $pageId) ->getQuery() ->getResult(); + + if ($result === []) { + return null; + } + + return $this->loadPageData($result); + } + + public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedResult + { + $queryBuilder = $this->createQueryBuilder('p'); + + $countQb = clone $queryBuilder; + $total = (int) $countQb + ->select('COUNT(DISTINCT p.id)') + ->getQuery() + ->getSingleScalarResult(); + + $rows = $queryBuilder + ->select('p AS page', 'd AS data') + ->leftJoin( + SubscribePageData::class, + 'd', + 'ON', + 'd.id = p.id' + ) + ->andWhere('p.id > :afterId') + ->setParameter('afterId', $filter->getLastId()) + ->orderBy('p.id', 'ASC') + ->setMaxResults($filter->getLimit()) + ->getQuery() + ->getResult(); + + $grouped = []; + foreach ($rows as $row) { + /** @var SubscribePage $page */ + $page = $row['page'] ?? null; + $data = $row['data'] ?? null; + if ($page !== null) { + $grouped[$page->getId()][] = $row; + } + if ($data !== null) { + $grouped[$data->getId()][] = ['data' => $data]; + } + } + + $pages = []; + foreach ($grouped as $group) { + $pages[] = $this->loadPageData($group); + } + + return new PaginatedResult( + items: array_values($pages), + total: $total, + limit: $filter->getLimit(), + lastId: $filter->getLastId(), + ); + } + + private function loadPageData(array $result): SubscribePage + { + /** @var SubscribePage $page */ + $page = array_shift($result)['page']; + + $data = []; + foreach ($result as $row) { + if (isset($row['data'])) { + $data[] = $row['data']; + } + } + $page->setData($data); + + return $page; } } diff --git a/src/Domain/Subscription/Service/Manager/SubscribePageManager.php b/src/Domain/Subscription/Service/Manager/SubscribePageManager.php index 4f9474d5..3a02e4cc 100644 --- a/src/Domain/Subscription/Service/Manager/SubscribePageManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscribePageManager.php @@ -5,24 +5,44 @@ namespace PhpList\Core\Domain\Subscription\Service\Manager; use Doctrine\ORM\EntityManagerInterface; +use LogicException; use PhpList\Core\Domain\Identity\Model\Administrator; use PhpList\Core\Domain\Subscription\Model\SubscribePage; use PhpList\Core\Domain\Subscription\Model\SubscribePageData; use PhpList\Core\Domain\Subscription\Repository\SubscriberPageDataRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriberPageRepository; -use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; -use Symfony\Contracts\Translation\TranslatorInterface; +use PhpList\Core\Domain\Subscription\Service\SubscribePageConfigMigrationService; +use Symfony\Component\DependencyInjection\Attribute\Autowire; class SubscribePageManager { public function __construct( private readonly SubscriberPageRepository $pageRepository, private readonly SubscriberPageDataRepository $pageDataRepository, + private readonly SubscribePageConfigMigrationService $configMigrationService, private readonly EntityManagerInterface $entityManager, - private readonly TranslatorInterface $translator, + #[Autowire('%parallel_use_with_phplist3%')] + private readonly bool $parallelUseWithPhpList3, ) { } + public function findPage(int $id): ?SubscribePage + { + $page = $this->pageRepository->findPageWithData($id); + if ($page === null) { + return null; + } + + if ($this->parallelUseWithPhpList3) { + $changed = $this->configMigrationService->copyToPageData($page); + if ($changed) { + $page = $this->pageRepository->findPageWithData($id) ?? $page; + } + } + + return $page; + } + public function createPage(string $title, bool $active = false, ?Administrator $owner = null): SubscribePage { $page = new SubscribePage(); @@ -35,15 +55,39 @@ public function createPage(string $title, bool $active = false, ?Administrator $ return $page; } - public function getPage(int $id): SubscribePage + public function syncPageData(array $data, SubscribePage $page): void { - /** @var SubscribePage|null $page */ - $page = $this->pageRepository->find($id); - if (!$page) { - throw new NotFoundHttpException($this->translator->trans('Subscribe page not found')); + if ($page->getId() === null) { + throw new LogicException('Page must be persisted before syncing data'); + } + $existingPageData = []; + foreach ($this->getPageData($page) as $pageData) { + $existingPageData[$pageData->getName()] = $pageData; } - return $page; + foreach ($data as $pageDataKey => $value) { + if (isset($existingPageData[$pageDataKey])) { + $pageData = $existingPageData[$pageDataKey]; + $pageData->setData($value); + unset($existingPageData[$pageDataKey]); + continue; + } + + $pageData = (new SubscribePageData()) + ->setId($page->getId()) + ->setName($pageDataKey) + ->setData($value); + + $this->pageDataRepository->persist($pageData); + } + + foreach ($existingPageData as $pageData) { + $this->entityManager->remove($pageData); + } + + if ($this->parallelUseWithPhpList3) { + $this->configMigrationService->copyToConfig(page: $page, data: $data); + } } public function updatePage( @@ -76,25 +120,8 @@ public function deletePage(SubscribePage $page): void } /** @return SubscribePageData[] */ - public function getPageData(SubscribePage $page): array + private function getPageData(SubscribePage $page): array { - return $this->pageDataRepository->getByPage($page,); - } - - public function setPageData(SubscribePage $page, string $name, ?string $value): SubscribePageData - { - /** @var SubscribePageData|null $data */ - $data = $this->pageDataRepository->findByPageAndName($page, $name); - - if (!$data) { - $data = (new SubscribePageData()) - ->setId((int)$page->getId()) - ->setName($name); - $this->entityManager->persist($data); - } - - $data->setData($value); - - return $data; + return $this->pageDataRepository->getByPage($page); } } diff --git a/src/Domain/Subscription/Service/SubscribePageConfigMigrationService.php b/src/Domain/Subscription/Service/SubscribePageConfigMigrationService.php new file mode 100644 index 00000000..6c0f3441 --- /dev/null +++ b/src/Domain/Subscription/Service/SubscribePageConfigMigrationService.php @@ -0,0 +1,165 @@ +getId(); + if ($pageId === null) { + return false; + } + + $configValues = $this->getConfigValues($pageId); + if ($configValues === []) { + return false; + } + + $existingData = $this->pageDataRepository->getByPage($page); + $existingDataByName = $this->indexPageDataByName($existingData); + + $hasChanges = $this->syncPageData( + $pageId, + $configValues, + $existingData, + $existingDataByName + ); + + $page->setData($existingData); + + if ($hasChanges) { + $this->entityManager->flush(); + } + + return $hasChanges; + } + + private function getConfigValues(int $pageId): array + { + $configValues = []; + + foreach (self::SUBSCRIBE_PAGE_SUFFIXES as $suffix) { + $value = $this->configRepository->findValueByItem($suffix . ':' . $pageId); + + if ($value !== null) { + $configValues[$suffix] = $value; + } + } + + return $configValues; + } + + private function indexPageDataByName(array $existingData): array + { + $indexed = []; + + foreach ($existingData as $pageData) { + $indexed[$pageData->getName()] = $pageData; + } + + return $indexed; + } + + private function syncPageData( + int $pageId, + array $configValues, + array &$existingData, + array $existingDataByName + ): bool { + $hasChanges = false; + + foreach ($configValues as $name => $value) { + if (isset($existingDataByName[$name])) { + $hasChanges = $this->updateExistingPageData( + $existingDataByName[$name], + $value + ) || $hasChanges; + + continue; + } + + $newPageData = (new SubscribePageData()) + ->setId($pageId) + ->setName($name) + ->setData($value); + + $this->pageDataRepository->persist($newPageData); + + $existingData[] = $newPageData; + $hasChanges = true; + } + + return $hasChanges; + } + + private function updateExistingPageData( + SubscribePageData $pageData, + string $value + ): bool { + if ($pageData->getData() === $value) { + return false; + } + + $pageData->setData($value); + + return true; + } + + public function copyToConfig(SubscribePage $page, array $data): void + { + $pageId = $page->getId(); + if ($pageId === null) { + return; + } + + foreach (self::SUBSCRIBE_PAGE_SUFFIXES as $suffix) { + if (!array_key_exists($suffix, $data)) { + continue; + } + + $value = $data[$suffix]; + if (!is_string($value) && $value !== null) { + continue; + } + + $configKey = $suffix . ':' . $pageId; + $config = $this->configRepository->findOneBy(['key' => $configKey]); + if (!$config instanceof Config) { + $config = (new Config()) + ->setKey($configKey) + ->setValue($value); + + $this->configRepository->persist($config); + continue; + } + + $config->setValue($value); + } + } +} diff --git a/tests/Unit/Domain/Configuration/Service/Provider/ConfigProviderTest.php b/tests/Unit/Domain/Configuration/Service/Provider/ConfigProviderTest.php index c5744908..ab6e90c5 100644 --- a/tests/Unit/Domain/Configuration/Service/Provider/ConfigProviderTest.php +++ b/tests/Unit/Domain/Configuration/Service/Provider/ConfigProviderTest.php @@ -51,29 +51,6 @@ private function pickNonBooleanCase(): ConfigOption $this->markTestSkipped('No non-boolean ConfigOption cases available to test.'); } - /** - * Utility: pick a namespaced case "parent:child" where parent exists as its own case. - */ - private function pickNamespacedCasePair(): array - { - $byValue = []; - foreach (ConfigOption::cases() as $c) { - $byValue[$c->value] = $c; - } - - foreach (ConfigOption::cases() as $c) { - if (!str_contains($c->value, ':')) { - continue; - } - [$parent] = explode(':', $c->value, 2); - if (isset($byValue[$parent])) { - return [$c, $byValue[$parent]]; - } - } - - $this->markTestSkipped('No namespaced ConfigOption (parent:child) pair found.'); - } - public function testIsEnabledRejectsNonBooleanKeys(): void { $nonBoolean = $this->pickNonBooleanCase(); @@ -248,32 +225,27 @@ public function testGetValueWithNamespacePrefersFullValue(): void $this->assertSame('FULL', $this->provider->getValueWithNamespace($key)); } - public function testGetValueWithNamespaceFallsBackToParentWhenFullEmpty(): void + public function testGetValueWithNamespaceSupportsStringNamespacedKey(): void { - [$child, $parent] = $this->pickNamespacedCasePair(); - - // Simulate: child is empty (null or ''), parent has value "PARENTVAL" $this->cache ->method('get') ->willReturnMap([ - ['cfg:' . $child->value, null], - ['cfg:' . $parent->value, 'PARENTVAL'], + ['cfg:subscribemessage:1', null], + ['cfg:subscribemessage', 'PARENT'], ]); - // child -> repo null; parent -> not consulted because cache returns value $this->repo ->method('findValueByItem') ->willReturnMap([ - [$child->value, null], + ['subscribemessage:1', null], + ['subscribemessage', 'PARENT'], ]); - // child miss is cached as null, parent value is not rewritten here (already cached) $this->cache ->expects($this->atLeastOnce()) - ->method('set'); - - $this->defaults->method('has')->willReturn(false); + ->method('set') + ->withAnyParameters(); - $this->assertSame('PARENTVAL', $this->provider->getValueWithNamespace($child)); + $this->assertSame('PARENT', $this->provider->getValueWithNamespace('subscribemessage:1')); } } diff --git a/tests/Unit/Domain/Identity/Repository/AdminAttributeValueRepositoryTest.php b/tests/Unit/Domain/Identity/Repository/AdminAttributeValueRepositoryTest.php index 99695df1..7f74c7b9 100644 --- a/tests/Unit/Domain/Identity/Repository/AdminAttributeValueRepositoryTest.php +++ b/tests/Unit/Domain/Identity/Repository/AdminAttributeValueRepositoryTest.php @@ -54,7 +54,11 @@ public function testFindOneByAdminIdAndAttributeId(): void $administrator = $this->createMock(Administrator::class); $administrator->method('getId')->willReturn($adminId); - $expectedResult = new AdminAttributeValue($attributeDefinition, $administrator, 'value'); + $expectedResult = new AdminAttributeValue( + $attributeDefinition, + $administrator, + 'value' + ); $this->entityManager->expects($this->once()) ->method('createQueryBuilder') @@ -70,13 +74,17 @@ public function testFindOneByAdminIdAndAttributeId(): void ->with(AdminAttributeValue::class, 'aav') ->willReturn($this->queryBuilder); + $joinCalls = []; + $this->queryBuilder->expects($this->exactly(2)) ->method('join') - ->withConsecutive( - ['aav.administrator', 'admin'], - ['aav.attributeDefinition', 'attr'] - ) - ->willReturn($this->queryBuilder); + ->willReturnCallback( + function (string $join, string $alias) use (&$joinCalls) { + $joinCalls[] = [$join, $alias]; + + return $this->queryBuilder; + } + ); $this->queryBuilder->expects($this->once()) ->method('where') @@ -88,13 +96,17 @@ public function testFindOneByAdminIdAndAttributeId(): void ->with('attr.id = :attributeId') ->willReturn($this->queryBuilder); + $setParameterCalls = []; + $this->queryBuilder->expects($this->exactly(2)) ->method('setParameter') - ->withConsecutive( - ['adminId', $adminId], - ['attributeId', $attributeId] - ) - ->willReturn($this->queryBuilder); + ->willReturnCallback( + function (string $key, mixed $value) use (&$setParameterCalls) { + $setParameterCalls[] = [$key, $value]; + + return $this->queryBuilder; + } + ); $this->queryBuilder->expects($this->once()) ->method('getQuery') @@ -104,7 +116,26 @@ public function testFindOneByAdminIdAndAttributeId(): void ->method('getOneOrNullResult') ->willReturn($expectedResult); - $result = $this->subject->findOneByAdminIdAndAttributeId($adminId, $attributeId); + $result = $this->subject->findOneByAdminIdAndAttributeId( + $adminId, + $attributeId + ); + + $this->assertSame( + [ + ['aav.administrator', 'admin'], + ['aav.attributeDefinition', 'attr'], + ], + $joinCalls, + ); + + $this->assertSame( + [ + ['adminId', $adminId], + ['attributeId', $attributeId], + ], + $setParameterCalls, + ); $this->assertSame($expectedResult, $result); } diff --git a/tests/Unit/Domain/Identity/Service/AdminCopyEmailSenderTest.php b/tests/Unit/Domain/Identity/Service/AdminCopyEmailSenderTest.php index 1173fac3..6f2e4cb4 100644 --- a/tests/Unit/Domain/Identity/Service/AdminCopyEmailSenderTest.php +++ b/tests/Unit/Domain/Identity/Service/AdminCopyEmailSenderTest.php @@ -108,44 +108,61 @@ public function testSendsToListOwnersWhenFlagEnabled(): void public function testFallsBackToAdminAddressesWhenNoOwnersOrFlagFalse(): void { $configProvider = $this->createMock(ConfigProvider::class); + $configProvider->method('isEnabled') ->with(ConfigOption::SendAdminCopies) ->willReturn(true); + $getValueCalls = []; + $configProvider->expects(self::exactly(2)) ->method('getValue') - ->withConsecutive([ConfigOption::AdminAddress], [ConfigOption::AdminAddresses]) - ->willReturnOnConsecutiveCalls( - 'single@example.com', - ' admin1@example.com, , admin2@example.com ,admin1@example.com ' + ->willReturnCallback( + function (ConfigOption $key) use (&$getValueCalls): string { + $getValueCalls[] = $key; + + return match ($key) { + ConfigOption::AdminAddress => 'single@example.com', + ConfigOption::AdminAddresses => + ' admin1@example.com, , admin2@example.com ,admin1@example.com ', + default => '', + }; + } ); - $expectedRecipients = ['admin1@example.com', 'admin2@example.com', 'single@example.com']; + $expectedRecipients = [ + 'admin1@example.com', + 'admin2@example.com', + 'single@example.com', + ]; $systemEmailBuilder = $this->createMock(SystemEmailBuilder::class); - $systemEmailBuilder->expects(self::exactly(count($expectedRecipients))) + + $buildCalls = []; + + $systemEmailBuilder + ->expects(self::exactly(count($expectedRecipients))) ->method('buildSystemEmail') - ->with(self::callback(function (MessagePrecacheDto $data): bool { - return str_starts_with($data->subject, 'phpList '); - })) - ->willReturn(new Email()); + ->willReturnCallback( + function (MessagePrecacheDto $data) use (&$buildCalls): Email { + $buildCalls[] = $data; + + return new Email(); + } + ); $mailer = $this->createMock(MailerInterface::class); + + $sendCalls = []; + $bounce = 'bounce@domain.test'; - $i = 0; + $mailer->expects(self::exactly(count($expectedRecipients))) ->method('send') - ->with( - self::isInstanceOf(Email::class), - self::callback(function (Envelope $envelope) use ($expectedRecipients, &$i, $bounce): bool { - $sender = $envelope->getSender(); - $recipient = $envelope->getRecipients()[0] ?? null; - $expected = $expectedRecipients[$i++] ?? null; - return $sender !== null - && $sender->getAddress() === $bounce - && $recipient !== null - && $recipient->getAddress() === $expected; - }) + ->willReturnCallback( + function (Email $email, Envelope $envelope) use (&$sendCalls): void { + $sendCalls[] = [$email, $envelope]; + } ); $sender = new AdminCopyEmailSender( @@ -153,14 +170,46 @@ public function testFallsBackToAdminAddressesWhenNoOwnersOrFlagFalse(): void systemEmailBuilder: $systemEmailBuilder, mailer: $mailer, logger: $this->createMock(LoggerInterface::class), - // ensure fallback path regardless of list owners sendListAdminCopy: false, bounceEmail: $bounce, ); - // Even if lists have owners, flag=false should ignore them and use AdminAddress(es) $listWithOwner = $this->createListWithOwner('ignored@example.com'); + $sender->__invoke('System Update', 'Body', [$listWithOwner]); + + $this->assertSame( + [ + ConfigOption::AdminAddress, + ConfigOption::AdminAddresses, + ], + $getValueCalls, + ); + + $this->assertCount(count($expectedRecipients), $buildCalls); + + foreach ($buildCalls as $call) { + $this->assertInstanceOf(MessagePrecacheDto::class, $call); + $this->assertStringStartsWith('phpList ', $call->subject); + } + + $this->assertCount(count($expectedRecipients), $sendCalls); + + foreach ($sendCalls as $index => [$email, $envelope]) { + $this->assertInstanceOf(Email::class, $email); + + $senderAddress = $envelope->getSender(); + $recipient = $envelope->getRecipients()[0] ?? null; + + $this->assertNotNull($senderAddress); + $this->assertSame($bounce, $senderAddress->getAddress()); + + $this->assertNotNull($recipient); + $this->assertSame( + $expectedRecipients[$index], + $recipient->getAddress() + ); + } } private function createListWithOwner(string $email): SubscriberList diff --git a/tests/Unit/Domain/Identity/Service/AdminNotifierTest.php b/tests/Unit/Domain/Identity/Service/AdminNotifierTest.php index 69510cc0..6b14e640 100644 --- a/tests/Unit/Domain/Identity/Service/AdminNotifierTest.php +++ b/tests/Unit/Domain/Identity/Service/AdminNotifierTest.php @@ -39,6 +39,7 @@ public function testNotifyForwardFailedSendsAdminCopyAndLogs(): void $lists = [new SubscriberList()]; $expectedSubject = 'Message Forwarded'; + $expectedMessage = sprintf( '%s tried forwarding message %d to %s but failed', $subscriber->getEmail(), @@ -46,27 +47,30 @@ public function testNotifyForwardFailedSendsAdminCopyAndLogs(): void $friendEmail ); - // Translator expectations: first for subject, then for message with placeholders + $translatorCalls = []; + $this->translator ->expects(self::exactly(2)) ->method('trans') - ->withConsecutive( - [$this->equalTo('Message Forwarded')], - [ - $this->equalTo('%subscriber% tried forwarding message %campaignId% to %email% but failed'), - $this->callback(function (array $params) use ($subscriber, $friendEmail): bool { - return ($params['%subscriber%'] ?? null) === $subscriber->getEmail() - && ($params['%campaignId%'] ?? null) === 42 - && ($params['%email%'] ?? null) === $friendEmail; - }) - ] - ) - ->willReturnOnConsecutiveCalls( - $expectedSubject, - $expectedMessage + ->willReturnCallback( + function ( + string $message, + array $params = [] + ) use ( + &$translatorCalls, + $expectedSubject, + $expectedMessage + ): string { + $translatorCalls[] = [$message, $params]; + + return match ($message) { + 'Message Forwarded' => $expectedSubject, + '%subscriber% tried forwarding message %campaignId% to %email% but failed' => $expectedMessage, + default => 'Unknown status encountered.', + }; + } ); - // Admin copy sender should be invoked with translated subject and message and same lists $this->adminCopyEmailSender ->expects(self::once()) ->method('__invoke') @@ -76,7 +80,6 @@ public function testNotifyForwardFailedSendsAdminCopyAndLogs(): void $this->identicalTo($lists) ); - // EventLogManager should log only on failure $this->eventLogManager ->expects(self::once()) ->method('log') @@ -97,8 +100,25 @@ public function testNotifyForwardFailedSendsAdminCopyAndLogs(): void friendEmail: $friendEmail, lists: $lists ); - } + $this->assertSame( + [ + [ + 'Message Forwarded', + [], + ], + [ + '%subscriber% tried forwarding message %campaignId% to %email% but failed', + [ + '%subscriber%' => $subscriber->getEmail(), + '%campaignId%' => 42, + '%email%' => $friendEmail, + ], + ], + ], + $translatorCalls, + ); + } public function testNotifyForwardSucceededSendsAdminCopyWithoutLogging(): void { $campaign = $this->createMock(Message::class); @@ -110,6 +130,7 @@ public function testNotifyForwardSucceededSendsAdminCopyWithoutLogging(): void $lists = [new SubscriberList(), new SubscriberList()]; $expectedSubject = 'Message Forwarded'; + $expectedMessage = sprintf( '%s has forwarded message %d to %s', $subscriber->getEmail(), @@ -117,23 +138,28 @@ public function testNotifyForwardSucceededSendsAdminCopyWithoutLogging(): void $friendEmail ); + $translatorCalls = []; + $this->translator ->expects(self::exactly(2)) ->method('trans') - ->withConsecutive( - [$this->equalTo('Message Forwarded')], - [ - $this->equalTo('%subscriber% has forwarded message %campaignId% to %email%'), - $this->callback(function (array $params) use ($subscriber, $friendEmail): bool { - return ($params['%subscriber%'] ?? null) === $subscriber->getEmail() - && ($params['%campaignId%'] ?? null) === 777 - && ($params['%email%'] ?? null) === $friendEmail; - }) - ] - ) - ->willReturnOnConsecutiveCalls( - $expectedSubject, - $expectedMessage + ->willReturnCallback( + function ( + string $message, + array $params = [] + ) use ( + &$translatorCalls, + $expectedSubject, + $expectedMessage + ): string { + $translatorCalls[] = [$message, $params]; + + return match ($message) { + 'Message Forwarded' => $expectedSubject, + '%subscriber% has forwarded message %campaignId% to %email%' => $expectedMessage, + default => 'Unknown status encountered.', + }; + } ); $this->adminCopyEmailSender @@ -161,5 +187,23 @@ public function testNotifyForwardSucceededSendsAdminCopyWithoutLogging(): void friendEmail: $friendEmail, lists: $lists ); + + $this->assertSame( + [ + [ + 'Message Forwarded', + [], + ], + [ + '%subscriber% has forwarded message %campaignId% to %email%', + [ + '%subscriber%' => $subscriber->getEmail(), + '%campaignId%' => 777, + '%email%' => $friendEmail, + ], + ], + ], + $translatorCalls, + ); } } diff --git a/tests/Unit/Domain/Identity/Service/PasswordManagerTest.php b/tests/Unit/Domain/Identity/Service/PasswordManagerTest.php index fd7aae53..c97547ff 100644 --- a/tests/Unit/Domain/Identity/Service/PasswordManagerTest.php +++ b/tests/Unit/Domain/Identity/Service/PasswordManagerTest.php @@ -203,18 +203,53 @@ public function testUpdatePasswordWithTokenUpdatesPasswordAndRemovesToken(): voi public function testCleanupExpiredTokensRemovesExpiredTokens(): void { $administrator = new Administrator(); - $expiredRequest1 = new AdminPasswordRequest(new DateTime('-1 day'), $administrator, 'token1'); - $expiredRequest2 = new AdminPasswordRequest(new DateTime('-2 days'), $administrator, 'token2'); - $validRequest = new AdminPasswordRequest(new DateTime('+1 day'), $administrator, 'token3'); - $this->passwordRequestRepository->expects($this->once()) + $expiredRequest1 = new AdminPasswordRequest( + new DateTime('-1 day'), + $administrator, + 'token1' + ); + + $expiredRequest2 = new AdminPasswordRequest( + new DateTime('-2 days'), + $administrator, + 'token2' + ); + + $validRequest = new AdminPasswordRequest( + new DateTime('+1 day'), + $administrator, + 'token3' + ); + + $this->passwordRequestRepository + ->expects($this->once()) ->method('findAll') - ->willReturn([$expiredRequest1, $expiredRequest2, $validRequest]); + ->willReturn([ + $expiredRequest1, + $expiredRequest2, + $validRequest, + ]); - $this->passwordRequestRepository->expects($this->exactly(2)) + $removedRequests = []; + + $this->passwordRequestRepository + ->expects($this->exactly(2)) ->method('remove') - ->withConsecutive([$expiredRequest1], [$expiredRequest2]); + ->willReturnCallback( + function (AdminPasswordRequest $request) use (&$removedRequests): void { + $removedRequests[] = $request; + } + ); $this->subject->cleanupExpiredTokens(); + + $this->assertSame( + [ + $expiredRequest1, + $expiredRequest2, + ], + $removedRequests, + ); } } diff --git a/tests/Unit/Domain/Identity/Service/SessionManagerTest.php b/tests/Unit/Domain/Identity/Service/SessionManagerTest.php index e655f4a5..ee1d0978 100644 --- a/tests/Unit/Domain/Identity/Service/SessionManagerTest.php +++ b/tests/Unit/Domain/Identity/Service/SessionManagerTest.php @@ -18,37 +18,69 @@ class SessionManagerTest extends TestCase public function testCreateSessionWithInvalidCredentialsThrowsExceptionAndLogs(): void { $adminRepo = $this->createMock(AdministratorRepository::class); + $adminRepo->expects(self::once()) ->method('findOneByLoginCredentials') ->with('admin', 'wrong') ->willReturn(null); $tokenRepo = $this->createMock(AdministratorTokenRepository::class); - $tokenRepo->expects(self::never())->method('save'); + + $tokenRepo->expects(self::never()) + ->method('save'); $eventLogManager = $this->createMock(EventLogManager::class); + $eventLogManager->expects(self::once()) ->method('log') ->with('login', $this->stringContains('admin')); + $translatorCalls = []; + $translator = $this->createMock(TranslatorInterface::class); + $translator->expects(self::exactly(2)) ->method('trans') - ->withConsecutive( - ["Failed admin login attempt for '%login%'", ['login' => 'admin']], - ['Not authorized', []] - ) - ->willReturnOnConsecutiveCalls( - "Failed admin login attempt for 'admin'", - 'Not authorized' + ->willReturnCallback( + function (string $message, array $parameters = []) use (&$translatorCalls): string { + $translatorCalls[] = [$message, $parameters]; + + return match ($message) { + "Failed admin login attempt for '%login%'" => + "Failed admin login attempt for 'admin'", + 'Not authorized' => 'Not authorized', + default => 'Unknown status encountered.', + }; + } ); - $manager = new SessionManager($tokenRepo, $adminRepo, $eventLogManager, $translator); + $manager = new SessionManager( + $tokenRepo, + $adminRepo, + $eventLogManager, + $translator + ); $this->expectException(UnauthorizedHttpException::class); $this->expectExceptionMessage('Not authorized'); - $manager->createSession('admin', 'wrong'); + try { + $manager->createSession('admin', 'wrong'); + } finally { + $this->assertSame( + [ + [ + "Failed admin login attempt for '%login%'", + ['login' => 'admin'], + ], + [ + 'Not authorized', + [], + ], + ], + $translatorCalls, + ); + } } public function testDeleteSessionCallsRemove(): void diff --git a/tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php b/tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php index 0d05bd7a..683c215a 100644 --- a/tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php +++ b/tests/Unit/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandlerTest.php @@ -333,26 +333,34 @@ public function testInvokeWithMailerException(): void public function testInvokeWithMultipleSubscribers(): void { $campaign = $this->createCampaignMock(); + $precached = new MessagePrecacheDto(); $precached->subject = 'Test Subject'; $precached->content = '

Test HTML message

'; $precached->textContent = 'Test text message'; $precached->footer = 'Test footer message'; + $metadata = $this->createMock(MessageMetadata::class); + $campaign->method('getMetadata')->willReturn($metadata); $campaign->method('getId')->willReturn(1); + $data = new CampaignProcessorMessage(1); - $this->messageRepository->method('findByIdAndStatus') + $this->messageRepository + ->method('findByIdAndStatus') ->with(1, MessageStatus::Submitted) ->willReturn($campaign); - $this->precacheService->expects($this->once()) + $this->precacheService + ->expects($this->once()) ->method('precacheMessage') ->with($campaign, $this->anything()) ->willReturn(true); - $this->cache->method('get')->willReturn($precached); + $this->cache + ->method('get') + ->willReturn($precached); $subscriber1 = $this->createMock(Subscriber::class); $subscriber1->method('getEmail')->willReturn('test1@example.com'); @@ -366,47 +374,94 @@ public function testInvokeWithMultipleSubscribers(): void $subscriber3->method('getEmail')->willReturn('invalid-email'); $subscriber3->method('getId')->willReturn(3); - $this->subscriberProvider->expects($this->once()) + $this->subscriberProvider + ->expects($this->once()) ->method('getSubscribersForMessageOrLists') ->with($data, $campaign) - ->willReturn([$subscriber1, $subscriber2, $subscriber3]); + ->willReturn([ + $subscriber1, + $subscriber2, + $subscriber3, + ]); - $this->messagePreparator->expects($this->exactly(2)) + $processMessageLinksCalls = []; + + $this->messagePreparator + ->expects($this->exactly(2)) ->method('processMessageLinks') - ->withConsecutive( - [1, $precached, $subscriber1], - [1, $precached, $subscriber2] - ) - ->willReturnOnConsecutiveCalls($precached, $precached); + ->willReturnCallback( + function ( + int $campaignId, + MessagePrecacheDto $dto, + Subscriber $subscriber + ) use ( + &$processMessageLinksCalls, + $precached + ): MessagePrecacheDto { + $processMessageLinksCalls[] = [ + $campaignId, + $dto, + $subscriber, + ]; + + return $precached; + } + ); - // Configure builder to return emails for first two subscribers $campaignEmailBuilder = (new ReflectionClass($this->handler)) ->getProperty('campaignEmailBuilder'); + /** @var EmailBuilder|MockObject $campaignBuilderMock */ $campaignBuilderMock = $campaignEmailBuilder->getValue($this->handler); - $campaignBuilderMock->expects($this->exactly(2)) + + $buildCampaignEmailCalls = []; + + $campaignBuilderMock + ->expects($this->exactly(2)) ->method('buildCampaignEmail') - ->willReturnOnConsecutiveCalls( - [ - (new Email())->to('test1@example.com')->subject('Test Subject')->text('x'), - OutputFormat::Text - ], - [ - (new Email())->to('test2@example.com')->subject('Test Subject')->text('x'), - OutputFormat::Text - ], + ->willReturnCallback( + function () use (&$buildCampaignEmailCalls): array { + $buildCampaignEmailCalls[] = func_get_args(); + + static $emails = [ + 'test1@example.com', + 'test2@example.com', + ]; + + $email = array_shift($emails); + + return [ + (new Email()) + ->to($email) + ->subject('Test Subject') + ->text('x'), + OutputFormat::Text, + ]; + } ); - $this->mailer->expects($this->exactly(2)) + $this->mailer + ->expects($this->exactly(2)) ->method('send'); $metadata->expects($this->atLeastOnce()) ->method('setStatus'); - $this->entityManager->expects($this->atLeastOnce()) + $this->entityManager + ->expects($this->atLeastOnce()) ->method('flush'); ($this->handler)($data); + + $this->assertSame( + [ + [1, $precached, $subscriber1], + [1, $precached, $subscriber2], + ], + $processMessageLinksCalls, + ); + + $this->assertCount(2, $buildCampaignEmailCalls); } /** diff --git a/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php b/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php index 71ea8d3f..df8afd18 100644 --- a/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php +++ b/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php @@ -155,29 +155,36 @@ public function testUserSpecificUrlReplacementAndExceptionOnEmpty(): void ->disableOriginalConstructor() ->onlyMethods(['getId', 'getEmail']) ->getMock(); + $subscriber->method('getId')->willReturn(55); $subscriber->method('getEmail')->willReturn('user@example.com'); + $this->subscriberRepository ->method('findOneByEmail') ->willReturn($subscriber); + $this->subscriberRepository ->method('getDataById') ->with(55) ->willReturn(['id' => 55]); - // Success path replacement - $dto = new MessagePrecacheDto(); - $dto->content = 'Intro [URL:example.com/path] End'; - $dto->userSpecificUrl = true; + $remotePageFetcherCalls = []; $this->remotePageFetcher ->expects($this->exactly(2)) ->method('__invoke') - ->withConsecutive( - ['https://example.com/path', ['id' => 55]], - ['https://example.com/empty', ['id' => 55]], - ) - ->willReturnOnConsecutiveCalls('
REMOTE
', ''); + ->willReturnCallback( + function (string $url, array $data) use (&$remotePageFetcherCalls): string { + $remotePageFetcherCalls[] = [$url, $data]; + + return match ($url) { + 'https://example.com/path' => '
REMOTE
', + 'https://example.com/empty' => '', + default => '
UNKNOWN
' + }; + } + ); + $this->placeholderProcessor ->method('process') ->willReturnCallback( @@ -187,10 +194,25 @@ static function (...$args): string { ); $builder = $this->makeBuilder(); + + $dto = new MessagePrecacheDto(); + $dto->content = 'Intro [URL:example.com/path] End'; + $dto->userSpecificUrl = true; + [$html] = $builder($dto, $subscriber, 11); - $this->assertStringContainsString('
REMOTE
', $html); - // Failure path (empty content) should log and throw + $this->assertSame( + [ + ['https://example.com/path', ['id' => 55]], + ], + [$remotePageFetcherCalls[0]], + ); + + $this->assertStringContainsString( + '
REMOTE
', + $html + ); + $dto2 = new MessagePrecacheDto(); $dto2->content = 'Again [URL:example.com/empty] test'; $dto2->userSpecificUrl = true; @@ -200,7 +222,18 @@ static function (...$args): string { ->method('log'); $this->expectException(RemotePageFetchException::class); - $builder($dto2, $subscriber, 12); + + try { + $builder($dto2, $subscriber, 12); + } finally { + $this->assertSame( + [ + ['https://example.com/path', ['id' => 55]], + ['https://example.com/empty', ['id' => 55]], + ], + $remotePageFetcherCalls, + ); + } } public function testTemplatePreventsDefaultStyleInjection(): void diff --git a/tests/Unit/Domain/Messaging/Service/Manager/BounceManagerTest.php b/tests/Unit/Domain/Messaging/Service/Manager/BounceManagerTest.php index 8dabc30e..445dd240 100644 --- a/tests/Unit/Domain/Messaging/Service/Manager/BounceManagerTest.php +++ b/tests/Unit/Domain/Messaging/Service/Manager/BounceManagerTest.php @@ -190,15 +190,25 @@ public function testGetUserMessageHistoryWithBouncesDelegates(): void public function testAnnounceDeletionModeLogsCorrectMessage(): void { + $infoCalls = []; + $this->logger->expects($this->exactly(2)) ->method('info') - ->withConsecutive([ - 'Running in test mode, not deleting messages from mailbox' - ], [ - 'Processed messages will be deleted from the mailbox' - ]); + ->willReturnCallback( + function (string $message) use (&$infoCalls): void { + $infoCalls[] = $message; + } + ); $this->manager->announceDeletionMode(true); $this->manager->announceDeletionMode(false); + + $this->assertSame( + [ + 'Running in test mode, not deleting messages from mailbox', + 'Processed messages will be deleted from the mailbox', + ], + $infoCalls, + ); } } diff --git a/tests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php b/tests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php index 8f7ee7f7..74d82447 100644 --- a/tests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php +++ b/tests/Unit/Domain/Messaging/Service/SystemMailConstructorTest.php @@ -137,7 +137,6 @@ public function testTemplateWithSignaturePlaceholderUsesPoweredByImageWhenFlagFa public function testTemplateWithoutSignatureAppendsPoweredByTextAndBeforeBodyEndWhenHtml(): void { - // Configure template usage with poweredByPhplist=true (use text snippet instead of image) $this->configProvider->method('getValue')->willReturnMap([ [ConfigOption::PoweredByText, 'PB'], [ConfigOption::SystemMessageTemplate, '11'], @@ -146,18 +145,32 @@ public function testTemplateWithoutSignatureAppendsPoweredByTextAndBeforeBodyEnd $template = new Template('sys-template'); $template->setContent('[CONTENT]'); $template->setText('[CONTENT]'); - $this->templateRepository->method('findOneById')->with(11)->willReturn($template); - $this->templateImageManager->method('parseLogoPlaceholders')->willReturnCallback(static fn ($h) => $h); + $this->templateRepository + ->method('findOneById') + ->with(11) + ->willReturn($template); - // Html2Text is called twice: once for the HTML message -> text, and once for powered-by text - $this->html2Text->expects($this->exactly(2)) + $this->templateImageManager + ->method('parseLogoPlaceholders') + ->willReturnCallback(static fn ($html) => $html); + + $html2TextCalls = []; + + $this->html2Text + ->expects($this->exactly(2)) ->method('__invoke') - ->withConsecutive( - ['Hello World'], - ['PB'] - ) - ->willReturnOnConsecutiveCalls('Hello World', 'PB'); + ->willReturnCallback( + function (string $html) use (&$html2TextCalls): ?string { + $html2TextCalls[] = $html; + + return match ($html) { + 'Hello World' => 'Hello World', + 'PB' => 'PB', + default => null, + }; + } + ); $constructor = new SystemMailContentBuilder( html2Text: $this->html2Text, @@ -166,17 +179,34 @@ public function testTemplateWithoutSignatureAppendsPoweredByTextAndBeforeBodyEnd templateImageManager: $this->templateImageManager, poweredByPhplist: true, ); + $dto = new MessagePrecacheDto(); $dto->subject = 'Sub'; $dto->content = 'Hello World'; [$html, $text] = $constructor($dto); - // HTML path: since poweredByPhplist=true, raw PoweredByText should be inserted before - $this->assertStringContainsString('Hello World', $html); - $this->assertMatchesRegularExpression('~PB\s*$~', $html); + $this->assertSame( + [ + 'Hello World', + 'PB', + ], + $html2TextCalls, + ); + + $this->assertStringContainsString( + 'Hello World', + $html + ); - // TEXT path: PoweredByText (converted) appended with two newlines since no [SIGNATURE] - $this->assertSame("Hello World\n\nPB", $text); + $this->assertMatchesRegularExpression( + '~PB\s*$~', + $html + ); + + $this->assertSame( + "Hello World\n\nPB", + $text + ); } } diff --git a/tests/Unit/Domain/Messaging/Service/TemplateImageEmbedderTest.php b/tests/Unit/Domain/Messaging/Service/TemplateImageEmbedderTest.php index 9190467a..643d9610 100644 --- a/tests/Unit/Domain/Messaging/Service/TemplateImageEmbedderTest.php +++ b/tests/Unit/Domain/Messaging/Service/TemplateImageEmbedderTest.php @@ -100,29 +100,57 @@ public function testExternalImagesEmbeddedAndSameHostLeftAlone(): void ]); $html = '

and ' - . '

'; + . '

'; - $this->externalImageService->expects($this->exactly(2)) + $cacheCalls = []; + + $this->externalImageService + ->expects($this->exactly(2)) ->method('cache') - ->withConsecutive( - ['https://cdn.other.org/pic.jpg', 111], - ['https://example.com/local.jpg', 111] - ) - ->willReturnOnConsecutiveCalls(true, false); + ->willReturnCallback( + function (string $url, int $messageId) use (&$cacheCalls): ?bool { + $cacheCalls[] = [$url, $messageId]; + + return match ($url) { + 'https://cdn.other.org/pic.jpg' => true, + 'https://example.com/local.jpg' => false, + default => null, + }; + } + ); $jpegBase64 = base64_encode('JPEGDATA'); - $this->externalImageService->expects($this->once()) + + $this->externalImageService + ->expects($this->once()) ->method('getFromCache') ->with('https://cdn.other.org/pic.jpg', 111) ->willReturn($jpegBase64); $embedder = $this->createEmbedder(embedExternal: true); + $out = $embedder($html, 111); + $this->assertSame( + [ + ['https://cdn.other.org/pic.jpg', 111], + ['https://example.com/local.jpg', 111], + ], + $cacheCalls, + ); + $this->assertStringContainsString('cid:', $out); - $this->assertStringContainsString('https://example.com/local.jpg', $out, 'Same-host URL should remain'); + + $this->assertStringContainsString( + 'https://example.com/local.jpg', + $out, + 'Same-host URL should remain' + ); + $this->assertCount(1, $embedder->attachment); + $att = $embedder->attachment[0]; + $this->assertSame('base64', $att[3]); $this->assertSame('image/jpeg', $att[4]); } diff --git a/tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php index 0c29242a..42003275 100644 --- a/tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php +++ b/tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php @@ -11,74 +11,144 @@ use PhpList\Core\Domain\Subscription\Repository\SubscriberPageDataRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriberPageRepository; use PhpList\Core\Domain\Subscription\Service\Manager\SubscribePageManager; +use PhpList\Core\Domain\Subscription\Service\SubscribePageConfigMigrationService; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; -use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; -use Symfony\Component\Translation\Translator; class SubscribePageManagerTest extends TestCase { private SubscriberPageRepository|MockObject $pageRepository; private SubscriberPageDataRepository|MockObject $pageDataRepository; + private SubscribePageConfigMigrationService|MockObject $configMigrationService; private EntityManagerInterface|MockObject $entityManager; private SubscribePageManager $manager; + private SubscribePage|MockObject $page; protected function setUp(): void { $this->pageRepository = $this->createMock(SubscriberPageRepository::class); $this->pageDataRepository = $this->createMock(SubscriberPageDataRepository::class); + $this->configMigrationService = $this->createMock(SubscribePageConfigMigrationService::class); $this->entityManager = $this->createMock(EntityManagerInterface::class); + $this->page = $this->createMock(SubscribePage::class); + $this->page->method('getId')->willReturn(1); - $this->manager = new SubscribePageManager( + $this->manager = $this->createManager(true); + } + + private function createManager(bool $parallelUseWithPhpList3): SubscribePageManager + { + return new SubscribePageManager( pageRepository: $this->pageRepository, pageDataRepository: $this->pageDataRepository, + configMigrationService: $this->configMigrationService, entityManager: $this->entityManager, - translator: new Translator('en'), + parallelUseWithPhpList3: $parallelUseWithPhpList3, ); } - public function testCreatePageCreatesAndSaves(): void + public function testFindPageReturnsPageFromRepositoryWithoutRefetchWhenMigrationMakesNoChanges(): void { - $owner = new Administrator(); + $page = new SubscribePage(); + $this->pageRepository ->expects($this->once()) - ->method('persist') - ->with($this->isInstanceOf(SubscribePage::class)); + ->method('findPageWithData') + ->with(123) + ->willReturn($page); - $page = $this->manager->createPage('My Page', true, $owner); + $this->configMigrationService + ->expects($this->once()) + ->method('copyToPageData') + ->with($page) + ->willReturn(false); - $this->assertInstanceOf(SubscribePage::class, $page); - $this->assertSame('My Page', $page->getTitle()); - $this->assertTrue($page->isActive()); - $this->assertSame($owner, $page->getOwner()); + $this->assertSame($page, $this->manager->findPage(123)); } - public function testGetPageReturnsPage(): void + public function testFindPageRefetchesWhenMigrationChangesPageData(): void { $page = new SubscribePage(); + $refetchedPage = new SubscribePage(); + + $findPageCalls = []; + $this->pageRepository + ->expects($this->exactly(2)) + ->method('findPageWithData') + ->willReturnCallback( + function (int $id) use ( + &$findPageCalls, + $page, + $refetchedPage + ): SubscribePage { + $findPageCalls[] = $id; + + return count($findPageCalls) === 1 + ? $page + : $refetchedPage; + } + ); + + $this->configMigrationService ->expects($this->once()) - ->method('find') - ->with(123) - ->willReturn($page); + ->method('copyToPageData') + ->with($page) + ->willReturn(true); + + $result = $this->manager->findPage(123); - $result = $this->manager->getPage(123); + $this->assertSame([123, 123], $findPageCalls); - $this->assertSame($page, $result); + $this->assertSame($refetchedPage, $result); } - public function testGetPageThrowsWhenNotFound(): void + public function testFindPageReturnsNullWhenMissing(): void { $this->pageRepository ->expects($this->once()) - ->method('find') - ->with(999) + ->method('findPageWithData') + ->with(123) ->willReturn(null); - $this->expectException(NotFoundHttpException::class); - $this->expectExceptionMessage('Subscribe page not found'); + $this->configMigrationService + ->expects($this->never()) + ->method('copyToPageData'); + + $this->assertNull($this->manager->findPage(123)); + } + + public function testFindPageSkipsConfigMigrationWhenFeatureIsDisabled(): void + { + $manager = $this->createManager(false); + + $this->pageRepository + ->expects($this->once()) + ->method('findPageWithData') + ->with(123) + ->willReturn($this->page); + + $this->configMigrationService + ->expects($this->never()) + ->method('copyToPageData'); + + $this->assertSame($this->page, $manager->findPage(123)); + } + + public function testCreatePageCreatesAndSaves(): void + { + $owner = new Administrator(); + $this->pageRepository + ->expects($this->once()) + ->method('persist') + ->with($this->isInstanceOf(SubscribePage::class)); - $this->manager->getPage(999); + $page = $this->manager->createPage('My Page', true, $owner); + + $this->assertInstanceOf(SubscribePage::class, $page); + $this->assertSame('My Page', $page->getTitle()); + $this->assertTrue($page->isActive()); + $this->assertSame($owner, $page->getOwner()); } public function testUpdatePageUpdatesProvidedFieldsAndFlushes(): void @@ -147,82 +217,231 @@ public function testDeletePageCallsRepositoryRemove(): void $this->manager->deletePage($page); } - public function testGetPageDataReturnsStringWhenFound(): void + public function testSyncPageDataWithEmptyExistingDataCreatesNewEntries(): void { - $page = new SubscribePage(); - $data = $this->createMock(SubscribePageData::class); - $data->expects($this->once())->method('getData')->willReturn('value'); + $this->pageDataRepository + ->expects($this->once()) + ->method('getByPage') + ->with($this->page) + ->willReturn([]); + + $this->pageDataRepository + ->expects($this->exactly(2)) + ->method('persist') + ->with($this->isInstanceOf(SubscribePageData::class)); + + $this->entityManager + ->expects($this->never()) + ->method('remove'); + + $data = [ + 'field1' => 'value1', + 'field2' => 'value2', + ]; + + $this->manager->syncPageData($data, $this->page); + } + + public function testSyncPageDataCallsCopyToConfigWhenFeatureIsEnabled(): void + { + $data = ['subscribemessage' => 'updated']; $this->pageDataRepository ->expects($this->once()) ->method('getByPage') - ->with($page) - ->willReturn([$data]); + ->with($this->page) + ->willReturn([]); + + $this->configMigrationService + ->expects($this->once()) + ->method('copyToConfig') + ->with(page: $this->page, data: $data); - $result = $this->manager->getPageData($page); - $this->assertIsArray($result); - $this->assertSame('value', $result[0]->getData()); + $this->manager->syncPageData($data, $this->page); } - public function testGetPageDataReturnsNullWhenNotFound(): void + public function testSyncPageDataSkipsCopyToConfigWhenFeatureIsDisabled(): void { - $page = new SubscribePage(); + $manager = $this->createManager(false); + $data = ['subscribemessage' => 'updated']; $this->pageDataRepository ->expects($this->once()) ->method('getByPage') - ->with($page) + ->with($this->page) ->willReturn([]); - $result = $this->manager->getPageData($page); - $this->assertEmpty($result); + $this->configMigrationService + ->expects($this->never()) + ->method('copyToConfig'); + + $manager->syncPageData($data, $this->page); } - public function testSetPageDataUpdatesExistingDataAndFlushes(): void + public function testSyncPageDataUpdatesExistingEntries(): void { - $page = new SubscribePage(); - $existing = new SubscribePageData(); - $existing->setId(5)->setName('color')->setData('red'); + $pageData1 = new SubscribePageData(); + $pageData1->setName('field1')->setData('old_value1'); + $pageData2 = new SubscribePageData(); + $pageData2->setName('field2')->setData('old_value2'); $this->pageDataRepository ->expects($this->once()) - ->method('findByPageAndName') - ->with($page, 'color') - ->willReturn($existing); + ->method('getByPage') + ->with($this->page) + ->willReturn([$pageData1, $pageData2]); - $this->entityManager + $this->pageDataRepository ->expects($this->never()) ->method('persist'); - $result = $this->manager->setPageData($page, 'color', 'blue'); + $this->entityManager + ->expects($this->never()) + ->method('remove'); + + $data = [ + 'field1' => 'new_value1', + 'field2' => 'new_value2', + ]; - $this->assertSame($existing, $result); - $this->assertSame('blue', $result->getData()); + $this->manager->syncPageData($data, $this->page); + + $this->assertSame('new_value1', $pageData1->getData()); + $this->assertSame('new_value2', $pageData2->getData()); } - public function testSetPageDataCreatesNewWhenMissingAndPersistsAndFlushes(): void + public function testSyncPageDataRemovesExistingEntriesNotInNewData(): void { - $page = $this->getMockBuilder(SubscribePage::class) - ->onlyMethods(['getId']) - ->getMock(); - $page->method('getId')->willReturn(123); + $pageData1 = new SubscribePageData(); + $pageData1->setName('field1')->setData('value1'); + $pageData2 = new SubscribePageData(); + $pageData2->setName('field2')->setData('value2'); $this->pageDataRepository ->expects($this->once()) - ->method('findByPageAndName') - ->with($page, 'greeting') - ->willReturn(null); + ->method('getByPage') + ->with($this->page) + ->willReturn([$pageData1, $pageData2]); + + $this->pageDataRepository + ->expects($this->never()) + ->method('persist'); $this->entityManager + ->expects($this->exactly(1)) + ->method('remove') + ->with($pageData2); + + $data = [ + 'field1' => 'value1', + ]; + + $this->manager->syncPageData($data, $this->page); + } + + public function testSyncPageDataWithMixedOperationsCreateUpdateDelete(): void + { + $existingData1 = new SubscribePageData(); + $existingData1->setName('keep_and_update')->setData('old_value'); + $existingData2 = new SubscribePageData(); + $existingData2->setName('to_delete')->setData('delete_me'); + + $this->pageDataRepository + ->expects($this->once()) + ->method('getByPage') + ->with($this->page) + ->willReturn([$existingData1, $existingData2]); + + $this->pageDataRepository ->expects($this->once()) ->method('persist') ->with($this->isInstanceOf(SubscribePageData::class)); - $result = $this->manager->setPageData($page, 'greeting', 'hello'); + $this->entityManager + ->expects($this->once()) + ->method('remove') + ->with($existingData2); + + $data = [ + 'keep_and_update' => 'updated_value', + 'new_field' => 'new_value', + ]; + + $this->manager->syncPageData($data, $this->page); + + $this->assertSame('updated_value', $existingData1->getData()); + } + + public function testSyncPageDataWithEmptyDataRemovesAllExistingEntries(): void + { + $page = $this->createMock(SubscribePage::class); + $page->method('getId')->willReturn(11); + + $pageData1 = new SubscribePageData(); + $pageData1->setName('field1')->setData('value1'); + + $pageData2 = new SubscribePageData(); + $pageData2->setName('field2')->setData('value2'); + + $this->pageDataRepository + ->expects($this->once()) + ->method('getByPage') + ->with($page) + ->willReturn([$pageData1, $pageData2]); + + $this->pageDataRepository + ->expects($this->never()) + ->method('persist'); + + $removedEntries = []; + + $this->entityManager + ->expects($this->exactly(2)) + ->method('remove') + ->willReturnCallback( + function (SubscribePageData $pageData) use (&$removedEntries): void { + $removedEntries[] = $pageData; + } + ); + + $data = []; + + $this->manager->syncPageData($data, $page); + + $this->assertSame( + [$pageData1, $pageData2], + $removedEntries, + ); + } + + public function testSyncPageDataPreservesExistingDataObjectsWhenKeepingEntries(): void + { + $originalPageData = new SubscribePageData(); + $originalPageData->setId(99)->setName('color')->setData('red'); + + $this->pageDataRepository + ->expects($this->once()) + ->method('getByPage') + ->with($this->page) + ->willReturn([$originalPageData]); + + $this->pageDataRepository + ->expects($this->never()) + ->method('persist'); + + $this->entityManager + ->expects($this->never()) + ->method('remove'); + + $data = [ + 'color' => 'blue', + ]; + + $this->manager->syncPageData($data, $this->page); - $this->assertInstanceOf(SubscribePageData::class, $result); - $this->assertSame(123, $result->getId()); - $this->assertSame('greeting', $result->getName()); - $this->assertSame('hello', $result->getData()); + // Should have updated the same object, not created a new one + $this->assertSame(99, $originalPageData->getId()); + $this->assertSame('color', $originalPageData->getName()); + $this->assertSame('blue', $originalPageData->getData()); } } diff --git a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberBlacklistManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberBlacklistManagerTest.php index a78bbc0b..cec3759d 100644 --- a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberBlacklistManagerTest.php +++ b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberBlacklistManagerTest.php @@ -98,15 +98,21 @@ public function testAddEmailToBlacklistAddsEntryAndReason(): void ->with('new@blacklist.com') ->willReturn(false); + $persistedEntities = []; + $this->entityManager ->expects($this->exactly(2)) ->method('persist') - ->withConsecutive( - [$this->isInstanceOf(UserBlacklist::class)], - [$this->isInstanceOf(UserBlacklistData::class)] - ); + ->willReturnCallback(function (object $entity) use (&$persistedEntities): void { + $persistedEntities[] = $entity; + }); $this->manager->addEmailToBlacklist('new@blacklist.com', 'test reason'); + + $this->assertCount(2, $persistedEntities); + + $this->assertInstanceOf(UserBlacklist::class, $persistedEntities[0]); + $this->assertInstanceOf(UserBlacklistData::class, $persistedEntities[1]); } public function testAddEmailToBlacklistAddsEntryWithoutReason(): void @@ -129,6 +135,7 @@ public function testRemoveEmailFromBlacklistRemovesAllRelatedData(): void { $blacklist = $this->createMock(UserBlacklist::class); $blacklistData = $this->createMock(UserBlacklistData::class); + $subscriber = $this->getMockBuilder(Subscriber::class) ->setConstructorArgs(['test@example.com']) ->onlyMethods(['setBlacklisted']) @@ -152,14 +159,27 @@ public function testRemoveEmailFromBlacklistRemovesAllRelatedData(): void ->with('remove@me.com') ->willReturn($subscriber); + $removedEntities = []; + $this->entityManager ->expects($this->exactly(2)) ->method('remove') - ->withConsecutive([$blacklist], [$blacklistData]); + ->willReturnCallback( + function (object $entity) use (&$removedEntities): void { + $removedEntities[] = $entity; + } + ); - $subscriber->expects($this->once())->method('setBlacklisted')->with(false); + $subscriber->expects($this->once()) + ->method('setBlacklisted') + ->with(false); $this->manager->removeEmailFromBlacklist('remove@me.com'); + + $this->assertSame( + [$blacklist, $blacklistData], + $removedEntities, + ); } public function testGetBlacklistReasonReturnsReasonOrNull(): void diff --git a/tests/Unit/Domain/Subscription/Service/SubscribePageConfigMigrationServiceTest.php b/tests/Unit/Domain/Subscription/Service/SubscribePageConfigMigrationServiceTest.php new file mode 100644 index 00000000..c4af2d7a --- /dev/null +++ b/tests/Unit/Domain/Subscription/Service/SubscribePageConfigMigrationServiceTest.php @@ -0,0 +1,264 @@ +configRepository = $this->createMock(ConfigRepository::class); + $this->pageDataRepository = $this->createMock(SubscriberPageDataRepository::class); + $this->entityManager = $this->createMock(EntityManagerInterface::class); + $this->service = new SubscribePageConfigMigrationService( + configRepository: $this->configRepository, + pageDataRepository: $this->pageDataRepository, + entityManager: $this->entityManager, + ); + } + + public function testCopyToPageDataSkipsPagesWithoutId(): void + { + $page = $this->getMockBuilder(SubscribePage::class) + ->onlyMethods(['getId']) + ->getMock(); + $page->method('getId')->willReturn(null); + + $this->configRepository + ->expects($this->never()) + ->method('findValueByItem'); + + $this->pageDataRepository + ->expects($this->never()) + ->method('getByPage'); + + $this->entityManager + ->expects($this->never()) + ->method('flush'); + + $this->assertFalse($this->service->copyToPageData($page)); + } + + public function testCopyToPageDataSkipsWhenNoConfigValuesFound(): void + { + $page = $this->getMockBuilder(SubscribePage::class) + ->onlyMethods(['getId']) + ->getMock(); + $page->method('getId')->willReturn(1); + + $this->configRepository + ->expects($this->exactly(6)) + ->method('findValueByItem') + ->willReturn(null); + + $this->pageDataRepository + ->expects($this->never()) + ->method('getByPage'); + + $this->entityManager + ->expects($this->never()) + ->method('flush'); + + $this->assertFalse($this->service->copyToPageData($page)); + } + + public function testCopyToPageDataSyncsExistingAndAddsMissingEntriesThenFlushes(): void + { + $page = $this->getMockBuilder(SubscribePage::class) + ->onlyMethods(['getId', 'setData']) + ->getMock(); + $page->method('getId')->willReturn(1); + $page->method('setData')->willReturnSelf(); + + $existing = (new SubscribePageData()) + ->setId(1) + ->setName('confirmationsubject') + ->setData('existing intro'); + + $this->configRepository + ->expects($this->exactly(6)) + ->method('findValueByItem') + ->willReturnMap([ + ['subscribemessage:1', 'message from config'], + ['subscribesubject:1', null], + ['confirmationsubject:1', 'updated subject'], + ['confirmationmessage:1', null], + ['unsubscribesubject:1', null], + ['unsubscribemessage:1', null], + ]); + + $this->pageDataRepository + ->expects($this->once()) + ->method('getByPage') + ->with($page) + ->willReturn([$existing]); + + $this->pageDataRepository + ->expects($this->once()) + ->method('persist') + ->with($this->callback(static function (SubscribePageData $pageData): bool { + return $pageData->getId() === 1 + && $pageData->getName() === 'subscribemessage' + && $pageData->getData() === 'message from config'; + })); + + $this->entityManager + ->expects($this->once()) + ->method('flush'); + + $page + ->expects($this->once()) + ->method('setData') + ->with($this->callback(static function (array $data): bool { + if (count($data) !== 2) { + return false; + } + + $dataByName = []; + foreach ($data as $item) { + if (!$item instanceof SubscribePageData) { + return false; + } + $dataByName[$item->getName()] = $item->getData(); + } + + return isset($dataByName['confirmationsubject'], $dataByName['subscribemessage']) + && $dataByName['confirmationsubject'] === 'updated subject' + && $dataByName['subscribemessage'] === 'message from config'; + })) + ->willReturnSelf(); + + $this->assertTrue($this->service->copyToPageData($page)); + } + + public function testCopyToConfigSkipsPagesWithoutId(): void + { + $page = $this->getMockBuilder(SubscribePage::class) + ->onlyMethods(['getId']) + ->getMock(); + $page->method('getId')->willReturn(null); + + $this->configRepository + ->expects($this->never()) + ->method('findOneBy'); + + $this->configRepository + ->expects($this->never()) + ->method('persist'); + + $this->service->copyToConfig($page, ['subscribemessage' => 'message']); + } + + public function testCopyToConfigSkipsUnsupportedDataKeys(): void + { + $page = $this->getMockBuilder(SubscribePage::class) + ->onlyMethods(['getId']) + ->getMock(); + $page->method('getId')->willReturn(10); + + $this->configRepository + ->expects($this->never()) + ->method('findOneBy'); + + $this->configRepository + ->expects($this->never()) + ->method('persist'); + + $this->service->copyToConfig($page, ['other_key' => 'value']); + } + + public function testCopyToConfigCreatesMissingConfigRows(): void + { + $page = $this->getMockBuilder(SubscribePage::class) + ->onlyMethods(['getId']) + ->getMock(); + + $page->method('getId')->willReturn(7); + + $findOneByCalls = []; + + $this->configRepository + ->expects($this->exactly(2)) + ->method('findOneBy') + ->willReturnCallback(function (array $criteria) use (&$findOneByCalls) { + $findOneByCalls[] = $criteria; + + return null; + }); + + $persistedConfigs = []; + + $this->configRepository + ->expects($this->exactly(2)) + ->method('persist') + ->willReturnCallback(function (Config $config) use (&$persistedConfigs): void { + $persistedConfigs[$config->getKey()] = $config->getValue(); + }); + + $this->service->copyToConfig( + $page, + [ + 'subscribemessage' => 'msg', + 'confirmationsubject' => 'subject', + ], + ); + + $this->assertSame( + [ + ['key' => 'subscribemessage:7'], + ['key' => 'confirmationsubject:7'], + ], + $findOneByCalls, + ); + + $this->assertSame( + [ + 'subscribemessage:7' => 'msg', + 'confirmationsubject:7' => 'subject', + ], + $persistedConfigs, + ); + } + + public function testCopyToConfigUpdatesExistingRowsWithoutPersist(): void + { + $page = $this->getMockBuilder(SubscribePage::class) + ->onlyMethods(['getId']) + ->getMock(); + $page->method('getId')->willReturn(11); + + $existing = (new Config()) + ->setKey('confirmationmessage:11') + ->setValue('old'); + + $this->configRepository + ->expects($this->once()) + ->method('findOneBy') + ->with(['key' => 'confirmationmessage:11']) + ->willReturn($existing); + + $this->configRepository + ->expects($this->never()) + ->method('persist'); + + $this->service->copyToConfig($page, ['confirmationmessage' => 'new']); + + $this->assertSame('new', $existing->getValue()); + } +} diff --git a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php index ef48183f..1b2a7bf6 100644 --- a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php +++ b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php @@ -63,14 +63,21 @@ public function testImportFromCsvCreatesNewSubscribers(): void $csvContent .= "another@example.com,0,0,1,1,\"More data\",Jane\n"; $tempFile = tempnam(sys_get_temp_dir(), 'csv_test'); + file_put_contents($tempFile, $csvContent); $uploadedFile = $this->createMock(UploadedFile::class); - $uploadedFile->method('getRealPath')->willReturn($tempFile); + + $uploadedFile->method('getRealPath') + ->willReturn($tempFile); $attributeDefinition = $this->createMock(SubscriberAttributeDefinition::class); - $attributeDefinition->method('getName')->willReturn('first_name'); - $attributeDefinition->method('getId')->willReturn(1); + + $attributeDefinition->method('getName') + ->willReturn('first_name'); + + $attributeDefinition->method('getId') + ->willReturn(1); $this->attributeDefinitionRepositoryMock ->method('findOneByName') @@ -94,6 +101,7 @@ public function testImportFromCsvCreatesNewSubscribers(): void htmlEmail: true, disabled: false, ); + $importDto1->extraData = 'Some extra data'; $importDto1->extraAttributes = ['first_name' => 'John']; @@ -104,6 +112,7 @@ public function testImportFromCsvCreatesNewSubscribers(): void htmlEmail: false, disabled: true ); + $importDto2->extraData = 'More data'; $importDto2->extraAttributes = ['first_name' => 'Jane']; @@ -112,25 +121,55 @@ public function testImportFromCsvCreatesNewSubscribers(): void ->with($tempFile) ->willReturn([ 'valid' => [$importDto1, $importDto2], - 'errors' => [] + 'errors' => [], ]); + $createdSubscribers = []; + $this->subscriberManagerMock ->expects($this->exactly(2)) ->method('createFromImport') - ->willReturnOnConsecutiveCalls($subscriber1, $subscriber2); + ->willReturnCallback( + function () use ( + &$createdSubscribers, + $subscriber1, + $subscriber2 + ): Subscriber { + $subscriber = count($createdSubscribers) === 0 + ? $subscriber1 + : $subscriber2; + + $createdSubscribers[] = $subscriber; + + return $subscriber; + } + ); + + $processedSubscribers = []; $this->attributeManagerMock ->expects($this->exactly(2)) ->method('processAttributes') - ->withConsecutive( - [$subscriber1], - [$subscriber2] + ->willReturnCallback( + function (Subscriber $subscriber) use (&$processedSubscribers): void { + $processedSubscribers[] = $subscriber; + } ); $options = new SubscriberImportOptions(); + $result = $this->subject->importFromCsv($uploadedFile, $options); + $this->assertSame( + [$subscriber1, $subscriber2], + $createdSubscribers, + ); + + $this->assertSame( + [$subscriber1, $subscriber2], + $processedSubscribers, + ); + $this->assertSame(2, $result['created']); $this->assertSame(0, $result['updated']); $this->assertSame(0, $result['skipped']);