From 6d971e989e8b0878878530e30bb8a2f228781524 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Wed, 8 Oct 2025 11:50:54 +0400 Subject: [PATCH 1/9] Add blacklisted stat to import result --- src/Domain/Subscription/Service/SubscriberCsvImporter.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Domain/Subscription/Service/SubscriberCsvImporter.php b/src/Domain/Subscription/Service/SubscriberCsvImporter.php index f5d9e535..f63a7303 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvImporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvImporter.php @@ -72,6 +72,7 @@ public function importFromCsv(UploadedFile $file, SubscriberImportOptions $optio 'created' => 0, 'updated' => 0, 'skipped' => 0, + 'blacklisted' => 0, 'errors' => [], ]; @@ -182,6 +183,10 @@ private function processRow( } } + if ($subscriber->isBlacklisted()) { + $stats['blacklisted']++; + } + $this->handleFlushAndEmail($subscriber, $options, $dto, $addedNewSubscriberToList); } From 60a681d1f106deb0bde87fa6c28d0319ae10fda7 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Wed, 8 Oct 2025 13:13:04 +0400 Subject: [PATCH 2/9] Add history --- .../Manager/SubscriberHistoryManager.php | 42 +++++++++++++++ .../Service/Manager/SubscriberManager.php | 8 ++- .../Service/SubscriberCsvImporter.php | 52 ++++++++++--------- 3 files changed, 75 insertions(+), 27 deletions(-) diff --git a/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php b/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php index bac2ef8d..21defbfc 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php @@ -6,6 +6,7 @@ use PhpList\Core\Domain\Common\ClientIpResolver; use PhpList\Core\Domain\Common\SystemInfoCollector; +use PhpList\Core\Domain\Identity\Model\Administrator; use PhpList\Core\Domain\Subscription\Model\Filter\SubscriberHistoryFilter; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberHistory; @@ -44,4 +45,45 @@ public function addHistory(Subscriber $subscriber, string $message, ?string $det return $subscriberHistory; } + + public function addHistoryFromImport( + Subscriber $subscriber, + array $listLines, + array $updatedData, + ?Administrator $admin = null, + ): void { + if (!$admin) { + $headerLine = 'API-v2-import - CLI: ' . PHP_EOL . PHP_EOL; + } else { + $headerLine = 'API-v2-import - adminId: ' . $admin->getId() . PHP_EOL . PHP_EOL; + } + + $lines = []; + + if (empty($updatedData) && empty($listLines)) { + $lines[] = 'No user details changed'; + } else { + $skip = ['password', 'modified']; + foreach ($updatedData as $field => [$old, $new]) { + if (in_array($field, $skip, true)) { + continue; + } + $lines[] = sprintf( + "%s = %s\n*changed* from %s", + $field, + json_encode($new), + json_encode($old) + ); + } + foreach ($listLines as $line) { + $lines[] = $line; + } + } + + $this->addHistory( + subscriber: $subscriber, + message: 'Import by ' . $admin->getLoginName(), + details: $headerLine . implode(PHP_EOL, $lines) . PHP_EOL + ); + } } diff --git a/src/Domain/Subscription/Service/Manager/SubscriberManager.php b/src/Domain/Subscription/Service/Manager/SubscriberManager.php index 59cd2505..9b35012b 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberManager.php @@ -136,7 +136,7 @@ public function createFromImport(ImportSubscriberDto $subscriberDto): Subscriber return $subscriber; } - public function updateFromImport(Subscriber $existingSubscriber, ImportSubscriberDto $subscriberDto): Subscriber + public function updateFromImport(Subscriber $existingSubscriber, ImportSubscriberDto $subscriberDto): array { $existingSubscriber->setEmail($subscriberDto->email); $existingSubscriber->setConfirmed($subscriberDto->confirmed); @@ -145,7 +145,11 @@ public function updateFromImport(Subscriber $existingSubscriber, ImportSubscribe $existingSubscriber->setDisabled($subscriberDto->disabled); $existingSubscriber->setExtraData($subscriberDto->extraData); - return $existingSubscriber; + $uow = $this->entityManager->getUnitOfWork(); + $meta = $this->entityManager->getClassMetadata(Subscriber::class); + $uow->computeChangeSet($meta, $existingSubscriber); + + return $uow->getEntityChangeSet($existingSubscriber); } public function decrementBounceCount(Subscriber $subscriber): void diff --git a/src/Domain/Subscription/Service/SubscriberCsvImporter.php b/src/Domain/Subscription/Service/SubscriberCsvImporter.php index f63a7303..1aadf9a6 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvImporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvImporter.php @@ -5,6 +5,7 @@ namespace PhpList\Core\Domain\Subscription\Service; use Doctrine\ORM\EntityManagerInterface; +use PhpList\Core\Domain\Identity\Model\Administrator; use PhpList\Core\Domain\Messaging\Message\SubscriptionConfirmationMessage; use PhpList\Core\Domain\Subscription\Exception\CouldNotReadUploadedFileException; use PhpList\Core\Domain\Subscription\Model\Dto\ImportSubscriberDto; @@ -13,6 +14,7 @@ use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeDefinitionRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberAttributeManager; +use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriptionManager; use Symfony\Component\HttpFoundation\File\UploadedFile; @@ -35,6 +37,7 @@ class SubscriberCsvImporter private EntityManagerInterface $entityManager; private TranslatorInterface $translator; private MessageBusInterface $messageBus; + private SubscriberHistoryManager $subscriberHistoryManager; public function __construct( SubscriberManager $subscriberManager, @@ -46,6 +49,7 @@ public function __construct( EntityManagerInterface $entityManager, TranslatorInterface $translator, MessageBusInterface $messageBus, + SubscriberHistoryManager $subscriberHistoryManager, ) { $this->subscriberManager = $subscriberManager; $this->attributeManager = $attributeManager; @@ -56,6 +60,7 @@ public function __construct( $this->entityManager = $entityManager; $this->translator = $translator; $this->messageBus = $messageBus; + $this->subscriberHistoryManager = $subscriberHistoryManager; } /** @@ -63,10 +68,10 @@ public function __construct( * * @param UploadedFile $file The uploaded CSV file * @param SubscriberImportOptions $options + * @param ?Administrator $admin * @return array Import statistics - * @throws CouldNotReadUploadedFileException When the uploaded file cannot be read during import */ - public function importFromCsv(UploadedFile $file, SubscriberImportOptions $options): array + public function importFromCsv(UploadedFile $file, SubscriberImportOptions $options, ?Administrator $admin = null): array { $stats = [ 'created' => 0, @@ -88,7 +93,7 @@ public function importFromCsv(UploadedFile $file, SubscriberImportOptions $optio foreach ($result['valid'] as $dto) { try { - $this->processRow($dto, $options, $stats); + $this->processRow($dto, $options, $stats, $admin); } catch (Throwable $e) { $stats['errors'][] = $this->translator->trans( 'Error processing %email%: %error%', @@ -118,11 +123,12 @@ public function importFromCsv(UploadedFile $file, SubscriberImportOptions $optio * @param UploadedFile $file The uploaded CSV file * @return array Import statistics */ - public function importAndUpdateFromCsv(UploadedFile $file, ?array $listIds = [], bool $dryRun = false): array + public function importAndUpdateFromCsv(UploadedFile $file, Administrator $admin, ?array $listIds = [], bool $dryRun = false): array { return $this->importFromCsv( file: $file, - options: new SubscriberImportOptions(updateExisting: true, listIds: $listIds, dryRun: $dryRun) + options: new SubscriberImportOptions(updateExisting: true, listIds: $listIds, dryRun: $dryRun), + admin: $admin, ); } @@ -132,25 +138,23 @@ public function importAndUpdateFromCsv(UploadedFile $file, ?array $listIds = [], * @param UploadedFile $file The uploaded CSV file * @return array Import statistics */ - public function importNewFromCsv(UploadedFile $file, ?array $listIds = [], bool $dryRun = false): array + public function importNewFromCsv(UploadedFile $file, Administrator $admin, ?array $listIds = [], bool $dryRun = false): array { return $this->importFromCsv( file: $file, - options: new SubscriberImportOptions(listIds: $listIds, dryRun: $dryRun) + options: new SubscriberImportOptions(listIds: $listIds, dryRun: $dryRun), + admin: $admin, ); } /** * Process a single row from the CSV file. - * - * @param ImportSubscriberDto $dto - * @param SubscriberImportOptions $options - * @param array $stats Statistics to update */ private function processRow( ImportSubscriberDto $dto, SubscriberImportOptions $options, array &$stats, + ?Administrator $admin = null ): void { if ($this->handleInvalidEmail($dto, $options, $stats)) { return; @@ -164,7 +168,7 @@ private function processRow( } if ($subscriber) { - $this->subscriberManager->updateFromImport($subscriber, $dto); + $updatedData = $this->subscriberManager->updateFromImport($subscriber, $dto); $stats['updated']++; } else { $subscriber = $this->subscriberManager->createFromImport($dto); @@ -174,11 +178,13 @@ private function processRow( $this->processAttributes($subscriber, $dto); $addedNewSubscriberToList = false; + $listLines = []; if (!$subscriber->isBlacklisted() && count($options->listIds) > 0) { foreach ($options->listIds as $listId) { $created = $this->subscriptionManager->addSubscriberToAList($subscriber, $listId); if ($created) { $addedNewSubscriberToList = true; + $listLines[] = sprintf('Subscribed to %s', $created->getSubscriberList()->getName()); } } } @@ -187,6 +193,7 @@ private function processRow( $stats['blacklisted']++; } + $this->subscriberHistoryManager->addHistoryFromImport($subscriber, $listLines, $updatedData ?? [], $admin); $this->handleFlushAndEmail($subscriber, $options, $dto, $addedNewSubscriberToList); } @@ -219,23 +226,18 @@ private function handleFlushAndEmail( if (!$options->dryRun) { $this->entityManager->flush(); if ($dto->sendConfirmation && $addedNewSubscriberToList) { - $this->sendSubscribeEmail($subscriber, $options->listIds); + $message = new SubscriptionConfirmationMessage( + email: $subscriber->getEmail(), + uniqueId: $subscriber->getUniqueId(), + listIds: $options->listIds, + htmlEmail: $subscriber->hasHtmlEmail(), + ); + + $this->messageBus->dispatch($message); } } } - private function sendSubscribeEmail(Subscriber $subscriber, array $listIds): void - { - $message = new SubscriptionConfirmationMessage( - email: $subscriber->getEmail(), - uniqueId: $subscriber->getUniqueId(), - listIds: $listIds, - htmlEmail: $subscriber->hasHtmlEmail(), - ); - - $this->messageBus->dispatch($message); - } - /** * Process subscriber attributes. * From 4af4d8fa7e87edab8c4892e477f1d3e0c96ee597 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Fri, 10 Oct 2025 12:03:16 +0400 Subject: [PATCH 3/9] Add translations --- resources/translations/messages.en.xlf | 16 ++++++ .../Manager/SubscriberHistoryManager.php | 27 +++++----- .../Service/SubscriberCsvImporter.php | 52 ++++++++++++++----- .../Manager/SubscriberHistoryManagerTest.php | 2 + .../Service/SubscriberCsvImporterTest.php | 2 + 5 files changed, 72 insertions(+), 27 deletions(-) diff --git a/resources/translations/messages.en.xlf b/resources/translations/messages.en.xlf index 5bfb12fd..a84aef99 100644 --- a/resources/translations/messages.en.xlf +++ b/resources/translations/messages.en.xlf @@ -684,6 +684,22 @@ Thank you. Footer of public pages __Footer of public pages + + Please confirm your subscription + __Please confirm your subscription + + + No user details changed + __No user details changed + + + %field% = %new% *changed* from %old% + __%field% = %new% *changed* from %old% + + + Subscribed to %list% + __Subscribed to %list% + diff --git a/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php b/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php index 21defbfc..f57dc7ce 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php @@ -11,21 +11,25 @@ use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberHistory; use PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryRepository; +use Symfony\Contracts\Translation\TranslatorInterface; class SubscriberHistoryManager { private SubscriberHistoryRepository $repository; private ClientIpResolver $clientIpResolver; private SystemInfoCollector $systemInfoCollector; + private TranslatorInterface $translator; public function __construct( SubscriberHistoryRepository $repository, ClientIpResolver $clientIpResolver, SystemInfoCollector $systemInfoCollector, + TranslatorInterface $translator, ) { $this->repository = $repository; $this->clientIpResolver = $clientIpResolver; $this->systemInfoCollector = $systemInfoCollector; + $this->translator = $translator; } public function getHistory(int $lastId, int $limit, SubscriberHistoryFilter $filter): array @@ -52,27 +56,24 @@ public function addHistoryFromImport( array $updatedData, ?Administrator $admin = null, ): void { - if (!$admin) { - $headerLine = 'API-v2-import - CLI: ' . PHP_EOL . PHP_EOL; - } else { - $headerLine = 'API-v2-import - adminId: ' . $admin->getId() . PHP_EOL . PHP_EOL; - } + $headerLine = sprintf("API-v2-import - %s: %s%s", $admin ? 'Admin' : 'CLI', $admin?->getId(), "\n\n"); $lines = []; - if (empty($updatedData) && empty($listLines)) { - $lines[] = 'No user details changed'; + $lines[] = $this->translator->trans('No user details changed'); } else { $skip = ['password', 'modified']; foreach ($updatedData as $field => [$old, $new]) { if (in_array($field, $skip, true)) { continue; } - $lines[] = sprintf( - "%s = %s\n*changed* from %s", - $field, - json_encode($new), - json_encode($old) + $lines[] = $this->translator->trans( + '%field% = %new% *changed* from %old%', + [ + '%field' => $field, + '%new%' => json_encode($new), + '%old%' => json_encode($old) + ], ); } foreach ($listLines as $line) { @@ -82,7 +83,7 @@ public function addHistoryFromImport( $this->addHistory( subscriber: $subscriber, - message: 'Import by ' . $admin->getLoginName(), + message: 'Import by ' . $admin?->getLoginName(), details: $headerLine . implode(PHP_EOL, $lines) . PHP_EOL ); } diff --git a/src/Domain/Subscription/Service/SubscriberCsvImporter.php b/src/Domain/Subscription/Service/SubscriberCsvImporter.php index 1aadf9a6..107e18a9 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvImporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvImporter.php @@ -25,6 +25,7 @@ /** * Service for importing subscribers from a CSV file. * @SuppressWarnings("CouplingBetweenObjects") + * @SuppressWarnings("ExcessiveParameterList") */ class SubscriberCsvImporter { @@ -65,14 +66,14 @@ public function __construct( /** * Import subscribers from a CSV file. - * - * @param UploadedFile $file The uploaded CSV file - * @param SubscriberImportOptions $options - * @param ?Administrator $admin * @return array Import statistics + * @throws CouldNotReadUploadedFileException */ - public function importFromCsv(UploadedFile $file, SubscriberImportOptions $options, ?Administrator $admin = null): array - { + public function importFromCsv( + UploadedFile $file, + SubscriberImportOptions $options, + ?Administrator $admin = null + ): array { $stats = [ 'created' => 0, 'updated' => 0, @@ -123,8 +124,12 @@ public function importFromCsv(UploadedFile $file, SubscriberImportOptions $optio * @param UploadedFile $file The uploaded CSV file * @return array Import statistics */ - public function importAndUpdateFromCsv(UploadedFile $file, Administrator $admin, ?array $listIds = [], bool $dryRun = false): array - { + public function importAndUpdateFromCsv( + UploadedFile $file, + Administrator $admin, + ?array $listIds = [], + bool $dryRun = false + ): array { return $this->importFromCsv( file: $file, options: new SubscriberImportOptions(updateExisting: true, listIds: $listIds, dryRun: $dryRun), @@ -138,8 +143,12 @@ public function importAndUpdateFromCsv(UploadedFile $file, Administrator $admin, * @param UploadedFile $file The uploaded CSV file * @return array Import statistics */ - public function importNewFromCsv(UploadedFile $file, Administrator $admin, ?array $listIds = [], bool $dryRun = false): array - { + public function importNewFromCsv( + UploadedFile $file, + Administrator $admin, + ?array $listIds = [], + bool $dryRun = false + ): array { return $this->importFromCsv( file: $file, options: new SubscriberImportOptions(listIds: $listIds, dryRun: $dryRun), @@ -161,9 +170,7 @@ private function processRow( } $subscriber = $this->subscriberRepository->findOneByEmail($dto->email); - if ($subscriber && !$options->updateExisting) { - $stats['skipped']++; - + if ($this->handleSkipCase($subscriber, $options, $stats)) { return; } @@ -184,7 +191,10 @@ private function processRow( $created = $this->subscriptionManager->addSubscriberToAList($subscriber, $listId); if ($created) { $addedNewSubscriberToList = true; - $listLines[] = sprintf('Subscribed to %s', $created->getSubscriberList()->getName()); + $listLines[] = $this->translator->trans( + 'Subscribed to %list%', + ['%list%' => $created->getSubscriberList()->getName()] + ); } } } @@ -217,6 +227,20 @@ private function handleInvalidEmail( return false; } + private function handleSkipCase( + ?Subscriber $existingSubscriber, + SubscriberImportOptions $options, + array &$stats + ): bool { + if ($existingSubscriber && !$options->updateExisting) { + $stats['skipped']++; + + return true; + } + + return false; + } + private function handleFlushAndEmail( Subscriber $subscriber, SubscriberImportOptions $options, diff --git a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberHistoryManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberHistoryManagerTest.php index 43ae2fcc..74da19c0 100644 --- a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberHistoryManagerTest.php +++ b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberHistoryManagerTest.php @@ -12,6 +12,7 @@ use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; +use Symfony\Contracts\Translation\TranslatorInterface; class SubscriberHistoryManagerTest extends TestCase { @@ -25,6 +26,7 @@ protected function setUp(): void repository: $this->subscriberHistoryRepository, clientIpResolver: $this->createMock(ClientIpResolver::class), systemInfoCollector: $this->createMock(SystemInfoCollector::class), + translator: $this->createMock(TranslatorInterface::class), ); } diff --git a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php index 1453cfa2..157bb48b 100644 --- a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php +++ b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php @@ -13,6 +13,7 @@ use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; use PhpList\Core\Domain\Subscription\Service\CsvImporter; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberAttributeManager; +use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriptionManager; use PhpList\Core\Domain\Subscription\Service\SubscriberCsvImporter; @@ -51,6 +52,7 @@ protected function setUp(): void entityManager: $entityManager, translator: new Translator('en'), messageBus: $this->createMock(MessageBusInterface::class), + subscriberHistoryManager: $this->createMock(SubscriberHistoryManager::class), ); } From 742ae52d8cb8024ec25ca9414136f8420734d6ae Mon Sep 17 00:00:00 2001 From: Tatevik Date: Fri, 10 Oct 2025 13:06:30 +0400 Subject: [PATCH 4/9] addHistory for unconfirmed --- resources/translations/messages.en.xlf | 10 +++++++++- .../Service/Processor/CampaignProcessor.php | 12 ++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/resources/translations/messages.en.xlf b/resources/translations/messages.en.xlf index a84aef99..0400eebd 100644 --- a/resources/translations/messages.en.xlf +++ b/resources/translations/messages.en.xlf @@ -692,7 +692,7 @@ Thank you. No user details changed __No user details changed - + %field% = %new% *changed* from %old% __%field% = %new% *changed* from %old% @@ -700,6 +700,14 @@ Thank you. Subscribed to %list% __Subscribed to %list% + + Subscriber marked unconfirmed for invalid email address + __Subscriber marked unconfirmed for invalid email address + + + Marked unconfirmed while sending campaign %message_id% + __Marked unconfirmed while sending campaign %message_id% + diff --git a/src/Domain/Messaging/Service/Processor/CampaignProcessor.php b/src/Domain/Messaging/Service/Processor/CampaignProcessor.php index a5deb074..7fa388ff 100644 --- a/src/Domain/Messaging/Service/Processor/CampaignProcessor.php +++ b/src/Domain/Messaging/Service/Processor/CampaignProcessor.php @@ -14,6 +14,7 @@ use PhpList\Core\Domain\Messaging\Service\RateLimitedCampaignMailer; use PhpList\Core\Domain\Messaging\Service\MaxProcessTimeLimiter; use PhpList\Core\Domain\Messaging\Service\MessageProcessingPreparator; +use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; use PhpList\Core\Domain\Subscription\Service\Provider\SubscriberProvider; use PhpList\Core\Domain\Subscription\Model\Subscriber; use Psr\Log\LoggerInterface; @@ -35,6 +36,7 @@ class CampaignProcessor private MaxProcessTimeLimiter $timeLimiter; private RequeueHandler $requeueHandler; private TranslatorInterface $translator; + private SubscriberHistoryManager $subscriberHistoryManager; public function __construct( RateLimitedCampaignMailer $mailer, @@ -46,6 +48,7 @@ public function __construct( MaxProcessTimeLimiter $timeLimiter, RequeueHandler $requeueHandler, TranslatorInterface $translator, + SubscriberHistoryManager $subscriberHistoryManager, ) { $this->mailer = $mailer; $this->entityManager = $entityManager; @@ -56,6 +59,7 @@ public function __construct( $this->timeLimiter = $timeLimiter; $this->requeueHandler = $requeueHandler; $this->translator = $translator; + $this->subscriberHistoryManager = $subscriberHistoryManager; } public function process(Message $campaign, ?OutputInterface $output = null): void @@ -89,6 +93,14 @@ public function process(Message $campaign, ?OutputInterface $output = null): voi $output?->writeln($this->translator->trans('Invalid email, marking unconfirmed: %email%', [ '%email%' => $subscriber->getEmail(), ])); + $this->subscriberHistoryManager->addHistory( + subscriber: $subscriber, + message: $this->translator->trans('Subscriber marked unconfirmed for invalid email address'), + details: $this->translator->trans( + 'Marked unconfirmed while sending campaign %message_id%', + ['%message_id%' => $campaign->getId()] + ) + ); continue; } From f976d469c7eb07dfa18006f2384271bed3aa4303 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Fri, 10 Oct 2025 15:40:25 +0400 Subject: [PATCH 5/9] Refactor --- config/services/providers.yml | 3 ++ resources/translations/messages.en.xlf | 4 +++ .../Manager/SubscriberAttributeManager.php | 27 ++++++++++++++ .../Manager/SubscriberHistoryManager.php | 32 ++++++++++++++--- .../Service/Manager/SubscriberManager.php | 16 +++++++-- .../Provider/SubscriberAttributeProvider.php | 28 +++++++++++++++ .../Service/SubscriberCsvImporter.php | 34 ++---------------- .../SubscriberAttributeManagerTest.php | 36 ++++++++++++++++--- .../Service/SubscriberCsvImporterTest.php | 11 +++--- 9 files changed, 139 insertions(+), 52 deletions(-) create mode 100644 src/Domain/Subscription/Service/Provider/SubscriberAttributeProvider.php diff --git a/config/services/providers.yml b/config/services/providers.yml index f4f06010..58c0278b 100644 --- a/config/services/providers.yml +++ b/config/services/providers.yml @@ -27,3 +27,6 @@ services: autowire: true arguments: $cache: '@Psr\SimpleCache\CacheInterface' + + PhpList\Core\Domain\Subscription\Service\Provider\SubscriberAttributeProvider: + autowire: true diff --git a/resources/translations/messages.en.xlf b/resources/translations/messages.en.xlf index 0400eebd..a0be84f6 100644 --- a/resources/translations/messages.en.xlf +++ b/resources/translations/messages.en.xlf @@ -708,6 +708,10 @@ Thank you. Marked unconfirmed while sending campaign %message_id% __Marked unconfirmed while sending campaign %message_id% + + Update by %admin% + __Update by %admin% + diff --git a/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php b/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php index 4446e0bf..eda1f769 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php @@ -9,21 +9,25 @@ use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeValue; +use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeDefinitionRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeValueRepository; use Symfony\Contracts\Translation\TranslatorInterface; class SubscriberAttributeManager { private SubscriberAttributeValueRepository $attributeRepository; + private SubscriberAttributeDefinitionRepository $attrDefinitionRepository; private EntityManagerInterface $entityManager; private TranslatorInterface $translator; public function __construct( SubscriberAttributeValueRepository $attributeRepository, + SubscriberAttributeDefinitionRepository $attrDefinitionRepository, EntityManagerInterface $entityManager, TranslatorInterface $translator, ) { $this->attributeRepository = $attributeRepository; + $this->attrDefinitionRepository = $attrDefinitionRepository; $this->entityManager = $entityManager; $this->translator = $translator; } @@ -60,4 +64,27 @@ public function delete(SubscriberAttributeValue $attribute): void { $this->attributeRepository->remove($attribute); } + + public function processAttributes(Subscriber $subscriber, array $extraData): void + { +// $oldAttributesMap = $this->subscriberAttributeProvider->getMappedValues($subscriber); + + foreach ($extraData as $key => $value) { + $lowerKey = strtolower((string)$key); + if (in_array($lowerKey, ['password', 'modified'], true)) { + continue; + } + + $attributeDefinition = $this->attrDefinitionRepository->findOneByName($key); + if ($attributeDefinition !== null) { + $this->createOrUpdate( + subscriber: $subscriber, + definition: $attributeDefinition, + value: $value + ); + } + } + +// $newAttributesMap = $this->subscriberAttributeProvider->getMappedValues($subscriber); + } } diff --git a/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php b/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php index f57dc7ce..4c0bc1e3 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php @@ -58,6 +58,32 @@ public function addHistoryFromImport( ): void { $headerLine = sprintf("API-v2-import - %s: %s%s", $admin ? 'Admin' : 'CLI', $admin?->getId(), "\n\n"); + $lines = $this->getHistoryLines($updatedData, $listLines); + + $this->addHistory( + subscriber: $subscriber, + message: 'Import by ' . $admin?->getLoginName(), + details: $headerLine . implode(PHP_EOL, $lines) . PHP_EOL + ); + } + + public function addHistoryFromApi( + Subscriber $subscriber, + array $listLines, + array $updatedData, + ?Administrator $admin = null, + ): void { + $lines = $this->getHistoryLines($updatedData, $listLines); + + $this->addHistory( + subscriber: $subscriber, + message: $this->translator->trans('Update by %admin%', ['%admin%' => $admin->getLoginName()]), + details: implode(PHP_EOL, $lines) . PHP_EOL + ); + } + + private function getHistoryLines(array $updatedData, array $listLines): array + { $lines = []; if (empty($updatedData) && empty($listLines)) { $lines[] = $this->translator->trans('No user details changed'); @@ -81,10 +107,6 @@ public function addHistoryFromImport( } } - $this->addHistory( - subscriber: $subscriber, - message: 'Import by ' . $admin?->getLoginName(), - details: $headerLine . implode(PHP_EOL, $lines) . PHP_EOL - ); + return $lines; } } diff --git a/src/Domain/Subscription/Service/Manager/SubscriberManager.php b/src/Domain/Subscription/Service/Manager/SubscriberManager.php index 9b35012b..535508e1 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberManager.php @@ -5,13 +5,13 @@ namespace PhpList\Core\Domain\Subscription\Service\Manager; use Doctrine\ORM\EntityManagerInterface; +use PhpList\Core\Domain\Identity\Model\Administrator; use PhpList\Core\Domain\Messaging\Message\SubscriberConfirmationMessage; use PhpList\Core\Domain\Subscription\Model\Dto\CreateSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Dto\ImportSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Dto\UpdateSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; -use PhpList\Core\Domain\Subscription\Service\SubscriberBlacklistService; use PhpList\Core\Domain\Subscription\Service\SubscriberDeletionService; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\Messenger\MessageBusInterface; @@ -24,19 +24,22 @@ class SubscriberManager private MessageBusInterface $messageBus; private SubscriberDeletionService $subscriberDeletionService; private TranslatorInterface $translator; + private SubscriberHistoryManager $subscriberHistoryManager; public function __construct( SubscriberRepository $subscriberRepository, EntityManagerInterface $entityManager, MessageBusInterface $messageBus, SubscriberDeletionService $subscriberDeletionService, - TranslatorInterface $translator + TranslatorInterface $translator, + SubscriberHistoryManager $subscriberHistoryManager, ) { $this->subscriberRepository = $subscriberRepository; $this->entityManager = $entityManager; $this->messageBus = $messageBus; $this->subscriberDeletionService = $subscriberDeletionService; $this->translator = $translator; + $this->subscriberHistoryManager = $subscriberHistoryManager; } public function createSubscriber(CreateSubscriberDto $subscriberDto): Subscriber @@ -74,7 +77,7 @@ public function getSubscriberById(int $subscriberId): ?Subscriber return $this->subscriberRepository->find($subscriberId); } - public function updateSubscriber(UpdateSubscriberDto $subscriberDto): Subscriber + public function updateSubscriber(UpdateSubscriberDto $subscriberDto, Administrator $admin): Subscriber { /** @var Subscriber $subscriber */ $subscriber = $this->subscriberRepository->find($subscriberDto->subscriberId); @@ -86,8 +89,15 @@ public function updateSubscriber(UpdateSubscriberDto $subscriberDto): Subscriber $subscriber->setDisabled($subscriberDto->disabled); $subscriber->setExtraData($subscriberDto->additionalData); + $uow = $this->entityManager->getUnitOfWork(); + $meta = $this->entityManager->getClassMetadata(Subscriber::class); + $uow->computeChangeSet($meta, $subscriber); + $changeSet = $uow->getEntityChangeSet($subscriber); + $this->entityManager->flush(); + $this->subscriberHistoryManager->addHistoryFromApi($subscriber, [], $changeSet, $admin); + return $subscriber; } diff --git a/src/Domain/Subscription/Service/Provider/SubscriberAttributeProvider.php b/src/Domain/Subscription/Service/Provider/SubscriberAttributeProvider.php new file mode 100644 index 00000000..eef62563 --- /dev/null +++ b/src/Domain/Subscription/Service/Provider/SubscriberAttributeProvider.php @@ -0,0 +1,28 @@ +attributesRepository->getForSubscriber($subscriber); + foreach ($userAttributes as $userAttribute) { + $data[$userAttribute->getAttributeDefinition()->getName()] = $this->resolver->resolve($userAttribute); + } + + return $data ?? []; + } +} diff --git a/src/Domain/Subscription/Service/SubscriberCsvImporter.php b/src/Domain/Subscription/Service/SubscriberCsvImporter.php index 107e18a9..9d4070bf 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvImporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvImporter.php @@ -11,7 +11,6 @@ use PhpList\Core\Domain\Subscription\Model\Dto\ImportSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Dto\SubscriberImportOptions; use PhpList\Core\Domain\Subscription\Model\Subscriber; -use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeDefinitionRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberAttributeManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; @@ -21,7 +20,7 @@ use Symfony\Component\Messenger\MessageBusInterface; use Symfony\Contracts\Translation\TranslatorInterface; use Throwable; - +// todo: check if dryRun will work (some function flush) /** * Service for importing subscribers from a CSV file. * @SuppressWarnings("CouplingBetweenObjects") @@ -34,7 +33,6 @@ class SubscriberCsvImporter private SubscriptionManager $subscriptionManager; private SubscriberRepository $subscriberRepository; private CsvImporter $csvImporter; - private SubscriberAttributeDefinitionRepository $attrDefinitionRepository; private EntityManagerInterface $entityManager; private TranslatorInterface $translator; private MessageBusInterface $messageBus; @@ -46,7 +44,6 @@ public function __construct( SubscriptionManager $subscriptionManager, SubscriberRepository $subscriberRepository, CsvImporter $csvImporter, - SubscriberAttributeDefinitionRepository $attrDefinitionRepository, EntityManagerInterface $entityManager, TranslatorInterface $translator, MessageBusInterface $messageBus, @@ -57,7 +54,6 @@ public function __construct( $this->subscriptionManager = $subscriptionManager; $this->subscriberRepository = $subscriberRepository; $this->csvImporter = $csvImporter; - $this->attrDefinitionRepository = $attrDefinitionRepository; $this->entityManager = $entityManager; $this->translator = $translator; $this->messageBus = $messageBus; @@ -182,7 +178,7 @@ private function processRow( $stats['created']++; } - $this->processAttributes($subscriber, $dto); + $this->attributeManager->processAttributes($subscriber, $dto->extraAttributes); $addedNewSubscriberToList = false; $listLines = []; @@ -261,30 +257,4 @@ private function handleFlushAndEmail( } } } - - /** - * Process subscriber attributes. - * - * @param Subscriber $subscriber The subscriber - * @param ImportSubscriberDto $dto - */ - private function processAttributes(Subscriber $subscriber, ImportSubscriberDto $dto): void - { - foreach ($dto->extraAttributes as $key => $value) { - $lowerKey = strtolower((string)$key); - // Do not import or update sensitive/system fields from CSV - if (in_array($lowerKey, ['password', 'modified'], true)) { - continue; - } - - $attributeDefinition = $this->attrDefinitionRepository->findOneByName($key); - if ($attributeDefinition !== null) { - $this->attributeManager->createOrUpdate( - subscriber: $subscriber, - definition: $attributeDefinition, - value: $value - ); - } - } - } } diff --git a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberAttributeManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberAttributeManagerTest.php index a827ab3f..332b3a7c 100644 --- a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberAttributeManagerTest.php +++ b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberAttributeManagerTest.php @@ -9,6 +9,7 @@ use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeValue; +use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeDefinitionRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeValueRepository; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberAttributeManager; use PHPUnit\Framework\TestCase; @@ -35,7 +36,12 @@ public function testCreateNewSubscriberAttribute(): void return $attr->getValue() === 'US'; })); - $manager = new SubscriberAttributeManager($subscriberAttrRepo, $entityManager, new Translator('en')); + $manager = new SubscriberAttributeManager( + attributeRepository: $subscriberAttrRepo, + attrDefinitionRepository: $this->createMock(SubscriberAttributeDefinitionRepository::class), + entityManager: $entityManager, + translator: new Translator('en') + ); $attribute = $manager->createOrUpdate($subscriber, $definition, 'US'); self::assertInstanceOf(SubscriberAttributeValue::class, $attribute); @@ -61,7 +67,12 @@ public function testUpdateExistingSubscriberAttribute(): void ->method('persist') ->with($existing); - $manager = new SubscriberAttributeManager($subscriberAttrRepo, $entityManager, new Translator('en')); + $manager = new SubscriberAttributeManager( + attributeRepository: $subscriberAttrRepo, + attrDefinitionRepository: $this->createMock(SubscriberAttributeDefinitionRepository::class), + entityManager: $entityManager, + translator: new Translator('en') + ); $result = $manager->createOrUpdate($subscriber, $definition, 'Updated'); self::assertSame('Updated', $result->getValue()); @@ -77,7 +88,12 @@ public function testCreateFailsWhenValueAndDefaultAreNull(): void $subscriberAttrRepo->method('findOneBySubscriberAndAttribute')->willReturn(null); - $manager = new SubscriberAttributeManager($subscriberAttrRepo, $entityManager, new Translator('en')); + $manager = new SubscriberAttributeManager( + attributeRepository: $subscriberAttrRepo, + attrDefinitionRepository: $this->createMock(SubscriberAttributeDefinitionRepository::class), + entityManager: $entityManager, + translator: new Translator('en') + ); $this->expectException(SubscriberAttributeCreationException::class); $this->expectExceptionMessage('Value is required'); @@ -96,7 +112,12 @@ public function testGetSubscriberAttribute(): void ->with(5, 10) ->willReturn($expected); - $manager = new SubscriberAttributeManager($subscriberAttrRepo, $entityManager, new Translator('en')); + $manager = new SubscriberAttributeManager( + attributeRepository: $subscriberAttrRepo, + attrDefinitionRepository: $this->createMock(SubscriberAttributeDefinitionRepository::class), + entityManager: $entityManager, + translator: new Translator('en') + ); $result = $manager->getSubscriberAttribute(5, 10); self::assertSame($expected, $result); @@ -112,7 +133,12 @@ public function testDeleteSubscriberAttribute(): void ->method('remove') ->with($attribute); - $manager = new SubscriberAttributeManager($subscriberAttrRepo, $entityManager, new Translator('en')); + $manager = new SubscriberAttributeManager( + attributeRepository: $subscriberAttrRepo, + attrDefinitionRepository: $this->createMock(SubscriberAttributeDefinitionRepository::class), + entityManager: $entityManager, + translator: new Translator('en') + ); $manager->delete($attribute); self::assertTrue(true); diff --git a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php index 157bb48b..50b0c879 100644 --- a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php +++ b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php @@ -48,7 +48,6 @@ protected function setUp(): void subscriptionManager: $subscriptionManagerMock, subscriberRepository: $this->subscriberRepositoryMock, csvImporter: $this->csvImporterMock, - attrDefinitionRepository: $this->attributeDefinitionRepositoryMock, entityManager: $entityManager, translator: new Translator('en'), messageBus: $this->createMock(MessageBusInterface::class), @@ -122,10 +121,10 @@ public function testImportFromCsvCreatesNewSubscribers(): void $this->attributeManagerMock ->expects($this->exactly(2)) - ->method('createOrUpdate') + ->method('processAttributes') ->withConsecutive( - [$subscriber1, $attributeDefinition, 'John'], - [$subscriber2, $attributeDefinition, 'Jane'] + [$subscriber1, $importDto1], + [$subscriber2, $importDto2] ); $options = new SubscriberImportOptions(); @@ -158,8 +157,6 @@ public function testImportFromCsvUpdatesExistingSubscribers(): void ->with('existing@example.com') ->willReturn($existingSubscriber); - $updatedSubscriber = $this->createMock(Subscriber::class); - $importDto = new ImportSubscriberDto( email: 'existing@example.com', confirmed: true, @@ -182,7 +179,7 @@ public function testImportFromCsvUpdatesExistingSubscribers(): void ->expects($this->once()) ->method('updateFromImport') ->with($existingSubscriber, $importDto) - ->willReturn($updatedSubscriber); + ->willReturn([]); $options = new SubscriberImportOptions(updateExisting: true); $result = $this->subject->importFromCsv($uploadedFile, $options); From b01f665d6d7bb059769c5df5a674e20a5a0efd91 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Sun, 12 Oct 2025 18:09:27 +0400 Subject: [PATCH 6/9] Add changeSetDto --- config/services/providers.yml | 2 +- resources/translations/messages.en.xlf | 14 ++++ .../Service/Processor/CampaignProcessor.php | 2 + .../Subscription/Model/Dto/ChangeSetDto.php | 81 +++++++++++++++++++ .../Manager/SubscriberAttributeManager.php | 10 +-- .../Manager/SubscriberHistoryManager.php | 51 +++++++++--- .../Service/Manager/SubscriberManager.php | 14 ++-- .../SubscriberAttributeChangeSetProvider.php | 81 +++++++++++++++++++ .../Provider/SubscriberAttributeProvider.php | 28 ------- .../Service/SubscriberCsvImporter.php | 14 +++- .../Processor/CampaignProcessorTest.php | 2 + .../Service/Manager/SubscriberManagerTest.php | 2 + .../Service/SubscriberCsvImporterTest.php | 15 +++- 13 files changed, 260 insertions(+), 56 deletions(-) create mode 100644 src/Domain/Subscription/Model/Dto/ChangeSetDto.php create mode 100644 src/Domain/Subscription/Service/Provider/SubscriberAttributeChangeSetProvider.php delete mode 100644 src/Domain/Subscription/Service/Provider/SubscriberAttributeProvider.php diff --git a/config/services/providers.yml b/config/services/providers.yml index 58c0278b..b7b66be8 100644 --- a/config/services/providers.yml +++ b/config/services/providers.yml @@ -28,5 +28,5 @@ services: arguments: $cache: '@Psr\SimpleCache\CacheInterface' - PhpList\Core\Domain\Subscription\Service\Provider\SubscriberAttributeProvider: + PhpList\Core\Domain\Subscription\Service\Provider\SubscriberAttributeChangeSetProvider: autowire: true diff --git a/resources/translations/messages.en.xlf b/resources/translations/messages.en.xlf index a0be84f6..9a9fae29 100644 --- a/resources/translations/messages.en.xlf +++ b/resources/translations/messages.en.xlf @@ -712,6 +712,20 @@ Thank you. Update by %admin% __Update by %admin% + + (no data) + __(no data) + + + %attribute% = %new_value% + changed from %old_value% + __%attribute% = %new_value% + changed from %old_value% + + + No data changed + __No data changed + diff --git a/src/Domain/Messaging/Service/Processor/CampaignProcessor.php b/src/Domain/Messaging/Service/Processor/CampaignProcessor.php index 7fa388ff..e16d4246 100644 --- a/src/Domain/Messaging/Service/Processor/CampaignProcessor.php +++ b/src/Domain/Messaging/Service/Processor/CampaignProcessor.php @@ -24,6 +24,8 @@ /** * @SuppressWarnings(PHPMD.CouplingBetweenObjects) + * @SuppressWarnings(PHPMD.StaticAccess) + * @SuppressWarnings(PHPMD.ExcessiveParameterList) */ class CampaignProcessor { diff --git a/src/Domain/Subscription/Model/Dto/ChangeSetDto.php b/src/Domain/Subscription/Model/Dto/ChangeSetDto.php new file mode 100644 index 00000000..ef3cc2da --- /dev/null +++ b/src/Domain/Subscription/Model/Dto/ChangeSetDto.php @@ -0,0 +1,81 @@ + + * + * Example: + * [ + * 'email' => [null, 'newemail@example.com'], + * 'isActive' => [true, false] + * ] + */ + private array $changes = []; + + /** + * @param array $changes + */ + public function __construct(array $changes = []) + { + $this->changes = $changes; + } + + /** + * @return array + */ + public function getChanges(): array + { + return $this->changes; + } + + public function hasChanges(): bool + { + return !empty($this->changes); + } + + public function hasField(string $field): bool + { + return array_key_exists($field, $this->changes); + } + + /** + * @return array{0: mixed, 1: mixed}|null + */ + public function getFieldChange(string $field): ?array + { + return $this->changes[$field] ?? null; + } + + /** + * @return mixed|null + */ + public function getOldValue(string $field): mixed + { + return $this->changes[$field][0] ?? null; + } + + /** + * @return mixed|null + */ + public function getNewValue(string $field): mixed + { + return $this->changes[$field][1] ?? null; + } + + public function toArray(): array + { + return $this->changes; + } + + public static function fromDoctrineChangeSet(array $changeSet): self + { + return new self($changeSet); + } +} diff --git a/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php b/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php index eda1f769..01524ba6 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php @@ -37,6 +37,8 @@ public function createOrUpdate( SubscriberAttributeDefinition $definition, ?string $value = null ): SubscriberAttributeValue { + // phpcs:ignore Generic.Commenting.Todo + // todo: clarify which attributes can be created/updated $subscriberAttribute = $this->attributeRepository ->findOneBySubscriberAndAttribute($subscriber, $definition); @@ -65,11 +67,9 @@ public function delete(SubscriberAttributeValue $attribute): void $this->attributeRepository->remove($attribute); } - public function processAttributes(Subscriber $subscriber, array $extraData): void + public function processAttributes(Subscriber $subscriber, array $attributeData): void { -// $oldAttributesMap = $this->subscriberAttributeProvider->getMappedValues($subscriber); - - foreach ($extraData as $key => $value) { + foreach ($attributeData as $key => $value) { $lowerKey = strtolower((string)$key); if (in_array($lowerKey, ['password', 'modified'], true)) { continue; @@ -84,7 +84,5 @@ public function processAttributes(Subscriber $subscriber, array $extraData): voi ); } } - -// $newAttributesMap = $this->subscriberAttributeProvider->getMappedValues($subscriber); } } diff --git a/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php b/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php index 4c0bc1e3..f8d35c0a 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php @@ -4,9 +4,11 @@ namespace PhpList\Core\Domain\Subscription\Service\Manager; +use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Domain\Common\ClientIpResolver; use PhpList\Core\Domain\Common\SystemInfoCollector; use PhpList\Core\Domain\Identity\Model\Administrator; +use PhpList\Core\Domain\Subscription\Model\Dto\ChangeSetDto; use PhpList\Core\Domain\Subscription\Model\Filter\SubscriberHistoryFilter; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberHistory; @@ -19,17 +21,20 @@ class SubscriberHistoryManager private ClientIpResolver $clientIpResolver; private SystemInfoCollector $systemInfoCollector; private TranslatorInterface $translator; + private EntityManagerInterface $entityManager; public function __construct( SubscriberHistoryRepository $repository, ClientIpResolver $clientIpResolver, SystemInfoCollector $systemInfoCollector, TranslatorInterface $translator, + EntityManagerInterface $entityManager, ) { $this->repository = $repository; $this->clientIpResolver = $clientIpResolver; $this->systemInfoCollector = $systemInfoCollector; $this->translator = $translator; + $this->entityManager = $entityManager; } public function getHistory(int $lastId, int $limit, SubscriberHistoryFilter $filter): array @@ -45,20 +50,47 @@ public function addHistory(Subscriber $subscriber, string $message, ?string $det $subscriberHistory->setSystemInfo($this->systemInfoCollector->collectAsString()); $subscriberHistory->setIp($this->clientIpResolver->resolve()); - $this->repository->save($subscriberHistory); + $this->entityManager->persist($subscriberHistory); return $subscriberHistory; } + public function addHistoryFromChangeSet( + Subscriber $subscriber, + string $message, + ChangeSetDto $changeSet, + ): SubscriberHistory { + $details = ''; + foreach ($changeSet->getChanges() as $attribute => [$old, $new]) { + if (in_array($attribute, ChangeSetDto::IGNORED_ATTRIBUTES, true) || $new === null) { + continue; + } + $details .= $this->translator->trans( + "%attribute% = %new_value% \n changed from %old_value%", + [ + '%attribute%' => $attribute, + '%new_value%' => $new, + '%old_value%' => $old ?? $this->translator->trans('(no data)'), + ] + ) . PHP_EOL; + } + + if ($details === '') { + $details .= $this->translator->trans('No data changed') . PHP_EOL; + } + + return $this->addHistory($subscriber, $message, $details); + } + public function addHistoryFromImport( Subscriber $subscriber, array $listLines, - array $updatedData, + ChangeSetDto $changeSetDto, ?Administrator $admin = null, ): void { - $headerLine = sprintf("API-v2-import - %s: %s%s", $admin ? 'Admin' : 'CLI', $admin?->getId(), "\n\n"); + $headerLine = sprintf('API-v2-import - %s: %s%s', $admin ? 'Admin' : 'CLI', $admin?->getId(), "\n\n"); - $lines = $this->getHistoryLines($updatedData, $listLines); + $lines = $this->getHistoryLines($changeSetDto, $listLines); $this->addHistory( subscriber: $subscriber, @@ -70,7 +102,7 @@ public function addHistoryFromImport( public function addHistoryFromApi( Subscriber $subscriber, array $listLines, - array $updatedData, + ChangeSetDto $updatedData, ?Administrator $admin = null, ): void { $lines = $this->getHistoryLines($updatedData, $listLines); @@ -82,15 +114,14 @@ public function addHistoryFromApi( ); } - private function getHistoryLines(array $updatedData, array $listLines): array + private function getHistoryLines(ChangeSetDto $updatedData, array $listLines): array { $lines = []; - if (empty($updatedData) && empty($listLines)) { + if (!$updatedData->hasChanges() && empty($listLines)) { $lines[] = $this->translator->trans('No user details changed'); } else { - $skip = ['password', 'modified']; - foreach ($updatedData as $field => [$old, $new]) { - if (in_array($field, $skip, true)) { + foreach ($updatedData->getChanges() as $field => [$old, $new]) { + if (in_array($field, ChangeSetDto::IGNORED_ATTRIBUTES, true)) { continue; } $lines[] = $this->translator->trans( diff --git a/src/Domain/Subscription/Service/Manager/SubscriberManager.php b/src/Domain/Subscription/Service/Manager/SubscriberManager.php index 535508e1..68076fc7 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberManager.php @@ -7,6 +7,7 @@ use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Domain\Identity\Model\Administrator; use PhpList\Core\Domain\Messaging\Message\SubscriberConfirmationMessage; +use PhpList\Core\Domain\Subscription\Model\Dto\ChangeSetDto; use PhpList\Core\Domain\Subscription\Model\Dto\CreateSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Dto\ImportSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Dto\UpdateSubscriberDto; @@ -17,6 +18,9 @@ use Symfony\Component\Messenger\MessageBusInterface; use Symfony\Contracts\Translation\TranslatorInterface; +/** + * @SuppressWarnings(PHPMD.CouplingBetweenObjects) + */ class SubscriberManager { private SubscriberRepository $subscriberRepository; @@ -77,6 +81,7 @@ public function getSubscriberById(int $subscriberId): ?Subscriber return $this->subscriberRepository->find($subscriberId); } + /** @SuppressWarnings(PHPMD.StaticAccess) */ public function updateSubscriber(UpdateSubscriberDto $subscriberDto, Administrator $admin): Subscriber { /** @var Subscriber $subscriber */ @@ -92,9 +97,7 @@ public function updateSubscriber(UpdateSubscriberDto $subscriberDto, Administrat $uow = $this->entityManager->getUnitOfWork(); $meta = $this->entityManager->getClassMetadata(Subscriber::class); $uow->computeChangeSet($meta, $subscriber); - $changeSet = $uow->getEntityChangeSet($subscriber); - - $this->entityManager->flush(); + $changeSet = ChangeSetDto::fromDoctrineChangeSet($uow->getEntityChangeSet($subscriber)); $this->subscriberHistoryManager->addHistoryFromApi($subscriber, [], $changeSet, $admin); @@ -146,7 +149,8 @@ public function createFromImport(ImportSubscriberDto $subscriberDto): Subscriber return $subscriber; } - public function updateFromImport(Subscriber $existingSubscriber, ImportSubscriberDto $subscriberDto): array + /** @SuppressWarnings(PHPMD.StaticAccess) */ + public function updateFromImport(Subscriber $existingSubscriber, ImportSubscriberDto $subscriberDto): ChangeSetDto { $existingSubscriber->setEmail($subscriberDto->email); $existingSubscriber->setConfirmed($subscriberDto->confirmed); @@ -159,7 +163,7 @@ public function updateFromImport(Subscriber $existingSubscriber, ImportSubscribe $meta = $this->entityManager->getClassMetadata(Subscriber::class); $uow->computeChangeSet($meta, $existingSubscriber); - return $uow->getEntityChangeSet($existingSubscriber); + return ChangeSetDto::fromDoctrineChangeSet($uow->getEntityChangeSet($existingSubscriber)); } public function decrementBounceCount(Subscriber $subscriber): void diff --git a/src/Domain/Subscription/Service/Provider/SubscriberAttributeChangeSetProvider.php b/src/Domain/Subscription/Service/Provider/SubscriberAttributeChangeSetProvider.php new file mode 100644 index 00000000..290c6de8 --- /dev/null +++ b/src/Domain/Subscription/Service/Provider/SubscriberAttributeChangeSetProvider.php @@ -0,0 +1,81 @@ + $attributeData + * @return ChangeSetDto + */ + public function getAttributeChangeSet(Subscriber $subscriber, array $attributeData): ChangeSetDto + { + $oldMap = $this->getMappedValues($subscriber); + + $canon = static function (array $attributes): array { + $out = []; + foreach ($attributes as $key => $value) { + $out[mb_strtolower((string)$key)] = $value; + } + return $out; + }; + + $oldC = $canon($oldMap); + $newC = $canon($attributeData); + + foreach (ChangeSetDto::IGNORED_ATTRIBUTES as $ignoredAttribute) { + $lowerCaseIgnoredAttribute = mb_strtolower($ignoredAttribute); + unset($oldC[$lowerCaseIgnoredAttribute], $newC[$lowerCaseIgnoredAttribute]); + } + + $keys = array_values(array_unique(array_merge(array_keys($oldC), array_keys($newC)))); + + $changeSet = []; + foreach ($keys as $key) { + $hasOld = array_key_exists($key, $oldC); + $hasNew = array_key_exists($key, $newC); + + if ($hasOld && !$hasNew) { + $changeSet[$key] = [$oldC[$key], null]; + continue; + } + + if (!$hasOld && $hasNew) { + $changeSet[$key] = [null, $newC[$key]]; + continue; + } + + if ($oldC[$key] !== $newC[$key]) { + $changeSet[$key] = [$oldC[$key], $newC[$key]]; + } + } + + return new ChangeSetDto($changeSet); + } + + private function getMappedValues(Subscriber $subscriber): array + { + $userAttributes = $this->attributesRepository->getForSubscriber($subscriber); + foreach ($userAttributes as $userAttribute) { + $data[$userAttribute->getAttributeDefinition()->getName()] = $this->resolver->resolve($userAttribute); + } + + return $data ?? []; + } +} diff --git a/src/Domain/Subscription/Service/Provider/SubscriberAttributeProvider.php b/src/Domain/Subscription/Service/Provider/SubscriberAttributeProvider.php deleted file mode 100644 index eef62563..00000000 --- a/src/Domain/Subscription/Service/Provider/SubscriberAttributeProvider.php +++ /dev/null @@ -1,28 +0,0 @@ -attributesRepository->getForSubscriber($subscriber); - foreach ($userAttributes as $userAttribute) { - $data[$userAttribute->getAttributeDefinition()->getName()] = $this->resolver->resolve($userAttribute); - } - - return $data ?? []; - } -} diff --git a/src/Domain/Subscription/Service/SubscriberCsvImporter.php b/src/Domain/Subscription/Service/SubscriberCsvImporter.php index 9d4070bf..4509d0ab 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvImporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvImporter.php @@ -8,6 +8,7 @@ use PhpList\Core\Domain\Identity\Model\Administrator; use PhpList\Core\Domain\Messaging\Message\SubscriptionConfirmationMessage; use PhpList\Core\Domain\Subscription\Exception\CouldNotReadUploadedFileException; +use PhpList\Core\Domain\Subscription\Model\Dto\ChangeSetDto; use PhpList\Core\Domain\Subscription\Model\Dto\ImportSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Dto\SubscriberImportOptions; use PhpList\Core\Domain\Subscription\Model\Subscriber; @@ -20,8 +21,10 @@ use Symfony\Component\Messenger\MessageBusInterface; use Symfony\Contracts\Translation\TranslatorInterface; use Throwable; -// todo: check if dryRun will work (some function flush) + /** + * phpcs:ignore Generic.Commenting.Todo + * @todo: check if dryRun will work (some function flush) * Service for importing subscribers from a CSV file. * @SuppressWarnings("CouplingBetweenObjects") * @SuppressWarnings("ExcessiveParameterList") @@ -171,7 +174,7 @@ private function processRow( } if ($subscriber) { - $updatedData = $this->subscriberManager->updateFromImport($subscriber, $dto); + $changeSet = $this->subscriberManager->updateFromImport($subscriber, $dto); $stats['updated']++; } else { $subscriber = $this->subscriberManager->createFromImport($dto); @@ -199,7 +202,12 @@ private function processRow( $stats['blacklisted']++; } - $this->subscriberHistoryManager->addHistoryFromImport($subscriber, $listLines, $updatedData ?? [], $admin); + $this->subscriberHistoryManager->addHistoryFromImport( + subscriber: $subscriber, + listLines: $listLines, + changeSetDto: $changeSet ?? new ChangeSetDto(), + admin: $admin + ); $this->handleFlushAndEmail($subscriber, $options, $dto, $addedNewSubscriberToList); } diff --git a/tests/Unit/Domain/Messaging/Service/Processor/CampaignProcessorTest.php b/tests/Unit/Domain/Messaging/Service/Processor/CampaignProcessorTest.php index e1976202..f2f82752 100644 --- a/tests/Unit/Domain/Messaging/Service/Processor/CampaignProcessorTest.php +++ b/tests/Unit/Domain/Messaging/Service/Processor/CampaignProcessorTest.php @@ -16,6 +16,7 @@ use PhpList\Core\Domain\Messaging\Service\Processor\CampaignProcessor; use PhpList\Core\Domain\Messaging\Service\RateLimitedCampaignMailer; use PhpList\Core\Domain\Subscription\Model\Subscriber; +use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; use PhpList\Core\Domain\Subscription\Service\Provider\SubscriberProvider; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -54,6 +55,7 @@ protected function setUp(): void timeLimiter: $this->createMock(MaxProcessTimeLimiter::class), requeueHandler: $this->createMock(RequeueHandler::class), translator: new Translator('en'), + subscriberHistoryManager: $this->createMock(SubscriberHistoryManager::class), ); } diff --git a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php index f96f32e2..0549b4de 100644 --- a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php +++ b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php @@ -9,6 +9,7 @@ use PhpList\Core\Domain\Subscription\Model\Dto\CreateSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; +use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberManager; use PhpList\Core\Domain\Subscription\Service\SubscriberDeletionService; use PHPUnit\Framework\MockObject\MockObject; @@ -37,6 +38,7 @@ protected function setUp(): void messageBus: $this->messageBus, subscriberDeletionService: $subscriberDeletionService, translator: new Translator('en'), + subscriberHistoryManager: $this->createMock(SubscriberHistoryManager::class) ); } diff --git a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php index 50b0c879..424a7e0d 100644 --- a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php +++ b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php @@ -5,6 +5,7 @@ namespace PhpList\Core\Tests\Unit\Domain\Subscription\Service; use Doctrine\ORM\EntityManagerInterface; +use PhpList\Core\Domain\Subscription\Model\Dto\ChangeSetDto; use PhpList\Core\Domain\Subscription\Model\Dto\ImportSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Dto\SubscriberImportOptions; use PhpList\Core\Domain\Subscription\Model\Subscriber; @@ -123,8 +124,8 @@ public function testImportFromCsvCreatesNewSubscribers(): void ->expects($this->exactly(2)) ->method('processAttributes') ->withConsecutive( - [$subscriber1, $importDto1], - [$subscriber2, $importDto2] + [$subscriber1], + [$subscriber2] ); $options = new SubscriberImportOptions(); @@ -179,7 +180,15 @@ public function testImportFromCsvUpdatesExistingSubscribers(): void ->expects($this->once()) ->method('updateFromImport') ->with($existingSubscriber, $importDto) - ->willReturn([]); + ->willReturn(new ChangeSetDto( + [ + 'extra_data' => [null, 'Updated data'], + 'confirmed' => [false, true], + 'html_email' => [false, true], + 'blacklisted' => [true, false], + 'disabled' => [true, false], + ] + )); $options = new SubscriberImportOptions(updateExisting: true); $result = $this->subject->importFromCsv($uploadedFile, $options); From 2c587aaf000325d5efe30261376bccafbeff37dc Mon Sep 17 00:00:00 2001 From: Tatevik Date: Sun, 12 Oct 2025 20:17:31 +0400 Subject: [PATCH 7/9] Do not send email on creating without any list --- .../Manager/SubscriberAttributeManager.php | 3 +- .../Service/Manager/SubscriberManager.php | 24 --------- .../Service/SubscriberCsvImporter.php | 50 ++++++++++++------- .../Service/Manager/SubscriberManagerTest.php | 36 ++----------- 4 files changed, 36 insertions(+), 77 deletions(-) diff --git a/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php b/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php index 01524ba6..75b9e3e2 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php @@ -6,6 +6,7 @@ use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Domain\Subscription\Exception\SubscriberAttributeCreationException; +use PhpList\Core\Domain\Subscription\Model\Dto\ChangeSetDto; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeValue; @@ -71,7 +72,7 @@ public function processAttributes(Subscriber $subscriber, array $attributeData): { foreach ($attributeData as $key => $value) { $lowerKey = strtolower((string)$key); - if (in_array($lowerKey, ['password', 'modified'], true)) { + if (in_array($lowerKey, ChangeSetDto::IGNORED_ATTRIBUTES, true)) { continue; } diff --git a/src/Domain/Subscription/Service/Manager/SubscriberManager.php b/src/Domain/Subscription/Service/Manager/SubscriberManager.php index 68076fc7..40bfcc20 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberManager.php @@ -6,7 +6,6 @@ use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Domain\Identity\Model\Administrator; -use PhpList\Core\Domain\Messaging\Message\SubscriberConfirmationMessage; use PhpList\Core\Domain\Subscription\Model\Dto\ChangeSetDto; use PhpList\Core\Domain\Subscription\Model\Dto\CreateSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Dto\ImportSubscriberDto; @@ -15,7 +14,6 @@ use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; use PhpList\Core\Domain\Subscription\Service\SubscriberDeletionService; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; -use Symfony\Component\Messenger\MessageBusInterface; use Symfony\Contracts\Translation\TranslatorInterface; /** @@ -25,7 +23,6 @@ class SubscriberManager { private SubscriberRepository $subscriberRepository; private EntityManagerInterface $entityManager; - private MessageBusInterface $messageBus; private SubscriberDeletionService $subscriberDeletionService; private TranslatorInterface $translator; private SubscriberHistoryManager $subscriberHistoryManager; @@ -33,14 +30,12 @@ class SubscriberManager public function __construct( SubscriberRepository $subscriberRepository, EntityManagerInterface $entityManager, - MessageBusInterface $messageBus, SubscriberDeletionService $subscriberDeletionService, TranslatorInterface $translator, SubscriberHistoryManager $subscriberHistoryManager, ) { $this->subscriberRepository = $subscriberRepository; $this->entityManager = $entityManager; - $this->messageBus = $messageBus; $this->subscriberDeletionService = $subscriberDeletionService; $this->translator = $translator; $this->subscriberHistoryManager = $subscriberHistoryManager; @@ -58,24 +53,9 @@ public function createSubscriber(CreateSubscriberDto $subscriberDto): Subscriber $this->subscriberRepository->save($subscriber); - if ($subscriberDto->requestConfirmation) { - $this->sendConfirmationEmail($subscriber); - } - return $subscriber; } - private function sendConfirmationEmail(Subscriber $subscriber): void - { - $message = new SubscriberConfirmationMessage( - email: $subscriber->getEmail(), - uniqueId:$subscriber->getUniqueId(), - htmlEmail: $subscriber->hasHtmlEmail() - ); - - $this->messageBus->dispatch($message); - } - public function getSubscriberById(int $subscriberId): ?Subscriber { return $this->subscriberRepository->find($subscriberId); @@ -142,10 +122,6 @@ public function createFromImport(ImportSubscriberDto $subscriberDto): Subscriber $this->entityManager->persist($subscriber); - if ($subscriberDto->sendConfirmation) { - $this->sendConfirmationEmail($subscriber); - } - return $subscriber; } diff --git a/src/Domain/Subscription/Service/SubscriberCsvImporter.php b/src/Domain/Subscription/Service/SubscriberCsvImporter.php index 4509d0ab..e80db165 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvImporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvImporter.php @@ -93,8 +93,22 @@ public function importFromCsv( foreach ($result['valid'] as $dto) { try { - $this->processRow($dto, $options, $stats, $admin); + $this->entityManager->beginTransaction(); + + $message = $this->processRow($dto, $options, $stats, $admin); + + if ($options->dryRun) { + $this->entityManager->rollback(); + } else { + $this->entityManager->flush(); + $this->entityManager->commit(); + if ($message !== null) { + $this->messageBus->dispatch($message); + } + } } catch (Throwable $e) { + $this->entityManager->rollback(); + $stats['errors'][] = $this->translator->trans( 'Error processing %email%: %error%', ['%email%' => $dto->email, '%error%' => $e->getMessage()] @@ -163,14 +177,14 @@ private function processRow( SubscriberImportOptions $options, array &$stats, ?Administrator $admin = null - ): void { + ): ?SubscriptionConfirmationMessage { if ($this->handleInvalidEmail($dto, $options, $stats)) { - return; + return null; } $subscriber = $this->subscriberRepository->findOneByEmail($dto->email); if ($this->handleSkipCase($subscriber, $options, $stats)) { - return; + return null; } if ($subscriber) { @@ -208,7 +222,8 @@ private function processRow( changeSetDto: $changeSet ?? new ChangeSetDto(), admin: $admin ); - $this->handleFlushAndEmail($subscriber, $options, $dto, $addedNewSubscriberToList); + + return $this->prepareConfirmationMessage($subscriber, $options, $dto, $addedNewSubscriberToList); } private function handleInvalidEmail( @@ -245,24 +260,21 @@ private function handleSkipCase( return false; } - private function handleFlushAndEmail( + private function prepareConfirmationMessage( Subscriber $subscriber, SubscriberImportOptions $options, ImportSubscriberDto $dto, bool $addedNewSubscriberToList - ): void { - if (!$options->dryRun) { - $this->entityManager->flush(); - if ($dto->sendConfirmation && $addedNewSubscriberToList) { - $message = new SubscriptionConfirmationMessage( - email: $subscriber->getEmail(), - uniqueId: $subscriber->getUniqueId(), - listIds: $options->listIds, - htmlEmail: $subscriber->hasHtmlEmail(), - ); - - $this->messageBus->dispatch($message); - } + ): ?SubscriptionConfirmationMessage { + if ($dto->sendConfirmation && $addedNewSubscriberToList) { + return new SubscriptionConfirmationMessage( + email: $subscriber->getEmail(), + uniqueId: $subscriber->getUniqueId(), + listIds: $options->listIds, + htmlEmail: $subscriber->hasHtmlEmail(), + ); } + + return null; } } diff --git a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php index 0549b4de..4f11c393 100644 --- a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php +++ b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php @@ -5,7 +5,6 @@ namespace PhpList\Core\Tests\Unit\Domain\Subscription\Service\Manager; use Doctrine\ORM\EntityManagerInterface; -use PhpList\Core\Domain\Messaging\Message\SubscriberConfirmationMessage; use PhpList\Core\Domain\Subscription\Model\Dto\CreateSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; @@ -14,28 +13,23 @@ use PhpList\Core\Domain\Subscription\Service\SubscriberDeletionService; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; -use Symfony\Component\Messenger\Envelope; -use Symfony\Component\Messenger\MessageBusInterface; use Symfony\Component\Translation\Translator; class SubscriberManagerTest extends TestCase { private SubscriberRepository|MockObject $subscriberRepository; private EntityManagerInterface|MockObject $entityManager; - private MessageBusInterface|MockObject $messageBus; private SubscriberManager $subscriberManager; protected function setUp(): void { $this->subscriberRepository = $this->createMock(SubscriberRepository::class); $this->entityManager = $this->createMock(EntityManagerInterface::class); - $this->messageBus = $this->createMock(MessageBusInterface::class); $subscriberDeletionService = $this->createMock(SubscriberDeletionService::class); $this->subscriberManager = new SubscriberManager( subscriberRepository: $this->subscriberRepository, entityManager: $this->entityManager, - messageBus: $this->messageBus, subscriberDeletionService: $subscriberDeletionService, translator: new Translator('en'), subscriberHistoryManager: $this->createMock(SubscriberHistoryManager::class) @@ -67,7 +61,7 @@ public function testCreateSubscriberPersistsAndReturnsProperlyInitializedEntity( $this->assertFalse($result->isDisabled()); } - public function testCreateSubscriberPersistsAndSendsEmail(): void + public function testCreateSubscriberPersists(): void { $this->subscriberRepository ->expects($this->once()) @@ -81,13 +75,6 @@ public function testCreateSubscriberPersistsAndSendsEmail(): void && $sub->isDisabled() === false; })); - $this->messageBus - ->expects($this->once()) - ->method('dispatch') - ->willReturnCallback(function ($message) { - return new Envelope($message); - }); - $dto = new CreateSubscriberDto(email: 'foo@bar.com', requestConfirmation: true, htmlEmail: true); $result = $this->subscriberManager->createSubscriber($dto); @@ -99,7 +86,7 @@ public function testCreateSubscriberPersistsAndSendsEmail(): void $this->assertFalse($result->isDisabled()); } - public function testCreateSubscriberWithConfirmationSendsConfirmationEmail(): void + public function testCreateSubscriberWithConfirmation(): void { $capturedSubscriber = null; $this->subscriberRepository @@ -111,19 +98,6 @@ public function testCreateSubscriberWithConfirmationSendsConfirmationEmail(): vo return true; })); - $this->messageBus - ->expects($this->once()) - ->method('dispatch') - ->with($this->callback(function (SubscriberConfirmationMessage $message) { - $this->assertEquals('test@example.com', $message->getEmail()); - $this->assertEquals('test-unique-id-123', $message->getUniqueId()); - $this->assertTrue($message->hasHtmlEmail()); - return true; - })) - ->willReturnCallback(function ($message) { - return new Envelope($message); - }); - $dto = new CreateSubscriberDto(email: 'test@example.com', requestConfirmation: true, htmlEmail: true); $this->subscriberManager->createSubscriber($dto); @@ -133,16 +107,12 @@ public function testCreateSubscriberWithConfirmationSendsConfirmationEmail(): vo $this->assertFalse($capturedSubscriber->isConfirmed()); } - public function testCreateSubscriberWithoutConfirmationDoesNotSendConfirmationEmail(): void + public function testCreateSubscriberWithoutConfirmation(): void { $this->subscriberRepository ->expects($this->once()) ->method('save'); - $this->messageBus - ->expects($this->never()) - ->method('dispatch'); - $dto = new CreateSubscriberDto(email: 'test@example.com', requestConfirmation: false, htmlEmail: true); $this->subscriberManager->createSubscriber($dto); } From 0be3b76325700f679cb8371f90e9eebf518ad8e7 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Sun, 12 Oct 2025 20:22:16 +0400 Subject: [PATCH 8/9] Flush in controller --- .../Service/Manager/SubscriptionManager.php | 12 ++++++++---- .../Service/Manager/SubscriberHistoryManagerTest.php | 2 ++ .../Service/Manager/SubscriptionManagerTest.php | 8 ++++++-- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/Domain/Subscription/Service/Manager/SubscriptionManager.php b/src/Domain/Subscription/Service/Manager/SubscriptionManager.php index c9921bab..b3631d91 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriptionManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriptionManager.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Domain\Subscription\Service\Manager; +use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Domain\Subscription\Exception\SubscriptionCreationException; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberList; @@ -19,17 +20,20 @@ class SubscriptionManager private SubscriberRepository $subscriberRepository; private SubscriberListRepository $subscriberListRepository; private TranslatorInterface $translator; + private EntityManagerInterface $entityManager; public function __construct( SubscriptionRepository $subscriptionRepository, SubscriberRepository $subscriberRepository, SubscriberListRepository $subscriberListRepository, - TranslatorInterface $translator + TranslatorInterface $translator, + EntityManagerInterface $entityManager ) { $this->subscriptionRepository = $subscriptionRepository; $this->subscriberRepository = $subscriberRepository; $this->subscriberListRepository = $subscriberListRepository; $this->translator = $translator; + $this->entityManager = $entityManager; } public function addSubscriberToAList(Subscriber $subscriber, int $listId): ?Subscription @@ -49,7 +53,7 @@ public function addSubscriberToAList(Subscriber $subscriber, int $listId): ?Subs $subscription->setSubscriber($subscriber); $subscription->setSubscriberList($subscriberList); - $this->subscriptionRepository->save($subscription); + $this->entityManager->persist($subscription); return $subscription; } @@ -83,7 +87,7 @@ private function createSubscription(SubscriberList $subscriberList, string $emai $subscription->setSubscriber($subscriber); $subscription->setSubscriberList($subscriberList); - $this->subscriptionRepository->save($subscription); + $this->entityManager->persist($subscription); return $subscription; } @@ -111,7 +115,7 @@ private function deleteSubscription(SubscriberList $subscriberList, string $emai throw new SubscriptionCreationException($message, 404); } - $this->subscriptionRepository->remove($subscription); + $this->entityManager->remove($subscription); } /** @return Subscriber[] */ diff --git a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberHistoryManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberHistoryManagerTest.php index 74da19c0..85e99730 100644 --- a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberHistoryManagerTest.php +++ b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberHistoryManagerTest.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Tests\Unit\Domain\Subscription\Service\Manager; +use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Domain\Common\ClientIpResolver; use PhpList\Core\Domain\Common\SystemInfoCollector; use PhpList\Core\Domain\Subscription\Model\Filter\SubscriberHistoryFilter; @@ -27,6 +28,7 @@ protected function setUp(): void clientIpResolver: $this->createMock(ClientIpResolver::class), systemInfoCollector: $this->createMock(SystemInfoCollector::class), translator: $this->createMock(TranslatorInterface::class), + entityManager: $this->createMock(EntityManagerInterface::class), ); } diff --git a/tests/Unit/Domain/Subscription/Service/Manager/SubscriptionManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/SubscriptionManagerTest.php index f0c1d3af..edbe1c07 100644 --- a/tests/Unit/Domain/Subscription/Service/Manager/SubscriptionManagerTest.php +++ b/tests/Unit/Domain/Subscription/Service/Manager/SubscriptionManagerTest.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Tests\Unit\Domain\Subscription\Service\Manager; +use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Domain\Subscription\Exception\SubscriptionCreationException; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberList; @@ -21,6 +22,7 @@ class SubscriptionManagerTest extends TestCase private SubscriptionRepository&MockObject $subscriptionRepository; private SubscriberRepository&MockObject $subscriberRepository; private TranslatorInterface&MockObject $translator; + private EntityManagerInterface&MockObject $entityManager; private SubscriptionManager $manager; protected function setUp(): void @@ -29,11 +31,13 @@ protected function setUp(): void $this->subscriberRepository = $this->createMock(SubscriberRepository::class); $subscriberListRepository = $this->createMock(SubscriberListRepository::class); $this->translator = $this->createMock(TranslatorInterface::class); + $this->entityManager = $this->createMock(EntityManagerInterface::class); $this->manager = new SubscriptionManager( subscriptionRepository: $this->subscriptionRepository, subscriberRepository: $this->subscriberRepository, subscriberListRepository: $subscriberListRepository, translator: $this->translator, + entityManager: $this->entityManager, ); } @@ -45,7 +49,7 @@ public function testCreateSubscriptionWhenSubscriberExists(): void $this->subscriberRepository->method('findOneBy')->with(['email' => $email])->willReturn($subscriber); $this->subscriptionRepository->method('findOneBySubscriberListAndSubscriber')->willReturn(null); - $this->subscriptionRepository->expects($this->once())->method('save'); + $this->entityManager->expects($this->once())->method('persist'); $subscriptions = $this->manager->createSubscriptions($list, [$email]); @@ -78,7 +82,7 @@ public function testDeleteSubscriptionSuccessfully(): void ->with($subscriberList->getId(), $email) ->willReturn($subscription); - $this->subscriptionRepository->expects($this->once())->method('remove')->with($subscription); + $this->entityManager->expects($this->once())->method('remove')->with($subscription); $this->manager->deleteSubscriptions($subscriberList, [$email]); } From 308eae9cbc8a072c23068dfedf6bc1e07372ac35 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 13 Oct 2025 10:51:52 +0400 Subject: [PATCH 9/9] Add test --- ...bscriberAttributeChangeSetProviderTest.php | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 tests/Unit/Domain/Subscription/Service/Provider/SubscriberAttributeChangeSetProviderTest.php diff --git a/tests/Unit/Domain/Subscription/Service/Provider/SubscriberAttributeChangeSetProviderTest.php b/tests/Unit/Domain/Subscription/Service/Provider/SubscriberAttributeChangeSetProviderTest.php new file mode 100644 index 00000000..a38baabf --- /dev/null +++ b/tests/Unit/Domain/Subscription/Service/Provider/SubscriberAttributeChangeSetProviderTest.php @@ -0,0 +1,175 @@ +resolver = $this->createMock(AttributeValueResolver::class); + $this->resolver + ->method('resolve') + ->willReturnCallback(function (SubscriberAttributeValue $attr) { + return $attr->getValue(); + }); + + $this->repository = $this->createMock(SubscriberAttributeValueRepository::class); + + $this->provider = new SubscriberAttributeChangeSetProvider( + resolver: $this->resolver, + attributesRepository: $this->repository, + ); + } + + public function testNoChangesWhenNewAndExistingAreIdenticalCaseInsensitive(): void + { + $subscriber = new Subscriber(); + $existing = [ + $this->attr('Name', 'John', $subscriber), + $this->attr('Age', '30', $subscriber), + ]; + + $this->repository->expects(self::once()) + ->method('getForSubscriber') + ->with($subscriber) + ->willReturn($existing); + + $newData = [ + 'name' => 'John', + 'AGE' => '30', + ]; + + $changeSet = $this->provider->getAttributeChangeSet($subscriber, $newData); + + self::assertInstanceOf(ChangeSetDto::class, $changeSet); + self::assertFalse($changeSet->hasChanges()); + self::assertSame([], $changeSet->getChanges()); + } + + public function testAddedAttributeAppearsWithNullOldValue(): void + { + $subscriber = new Subscriber(); + $existing = [ + $this->attr('Name', 'John', $subscriber), + ]; + + $this->repository->method('getForSubscriber')->willReturn($existing); + + $newData = [ + 'name' => 'John', + 'city' => 'NY', + ]; + + $changeSet = $this->provider->getAttributeChangeSet($subscriber, $newData); + + self::assertTrue($changeSet->hasField('city')); + self::assertSame([null, 'NY'], $changeSet->getFieldChange('city')); + + self::assertSame(['city' => [null, 'NY']], $changeSet->getChanges()); + } + + public function testRemovedAttributeAppearsWithNullNewValue(): void + { + $subscriber = new Subscriber(); + $existing = [ + $this->attr('Country', 'US', $subscriber), + ]; + + $this->repository->method('getForSubscriber')->willReturn($existing); + + $changeSet = $this->provider->getAttributeChangeSet($subscriber, []); + + self::assertTrue($changeSet->hasField('country')); + self::assertSame(['US', null], $changeSet->getFieldChange('country')); + self::assertSame(['country' => ['US', null]], $changeSet->getChanges()); + } + + public function testChangedAttributeShowsOldAndNewValues(): void + { + $subscriber = new Subscriber(); + $existing = [ + $this->attr('Phone', '123', $subscriber), + ]; + + $this->repository->method('getForSubscriber')->willReturn($existing); + + $newData = [ + 'phone' => '456', + ]; + + $changeSet = $this->provider->getAttributeChangeSet($subscriber, $newData); + + self::assertSame(['123', '456'], $changeSet->getFieldChange('phone')); + self::assertSame(['phone' => ['123', '456']], $changeSet->getChanges()); + } + + public function testIgnoredAttributesAreExcluded(): void + { + $subscriber = new Subscriber(); + $existing = [ + $this->attr('Password', 'old', $subscriber), + $this->attr('Modified', 'yesterday', $subscriber), + $this->attr('Nickname', 'Bob', $subscriber), + ]; + + $this->repository->method('getForSubscriber')->willReturn($existing); + + $newData = [ + 'password' => 'new', + 'MODIFIED' => null, + 'nickname' => 'Bobby', + ]; + + $changeSet = $this->provider->getAttributeChangeSet($subscriber, $newData); + + self::assertFalse($changeSet->hasField('password')); + self::assertFalse($changeSet->hasField('modified')); + self::assertTrue($changeSet->hasField('nickname')); + self::assertSame(['Bob', 'Bobby'], $changeSet->getFieldChange('nickname')); + self::assertSame(['nickname' => ['Bob', 'Bobby']], $changeSet->getChanges()); + } + + public function testCaseInsensitiveKeyComparisonAndResultLowercasing(): void + { + $subscriber = new Subscriber(); + $existing = [ + $this->attr('FirstName', 'Ann', $subscriber), + ]; + + $this->repository->method('getForSubscriber')->willReturn($existing); + + $newData = [ + 'firstname' => 'Anna', + ]; + + $changeSet = $this->provider->getAttributeChangeSet($subscriber, $newData); + + self::assertTrue($changeSet->hasField('firstname')); + self::assertSame(['Ann', 'Anna'], $changeSet->getFieldChange('firstname')); + self::assertSame(['firstname' => ['Ann', 'Anna']], $changeSet->getChanges()); + } + + private function attr(string $name, ?string $value, Subscriber $subscriber): SubscriberAttributeValue + { + $def = (new SubscriberAttributeDefinition())->setName($name); + $attr = new SubscriberAttributeValue($def, $subscriber); + $attr->setValue($value); + return $attr; + } +}