diff --git a/resources/translations/messages.en.xlf b/resources/translations/messages.en.xlf index b3204742..efeb8ad2 100644 --- a/resources/translations/messages.en.xlf +++ b/resources/translations/messages.en.xlf @@ -730,6 +730,10 @@ Thank you. Campaign not found or not in submitted status __Campaign not found or not in submitted status + + Conflict: email and foreign key refer to different subscribers. + __Conflict: email and foreign key refer to different subscribers. + diff --git a/src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php b/src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php index 425a57d6..ee468fa1 100644 --- a/src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php +++ b/src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php @@ -11,6 +11,12 @@ class ImportSubscriberDto #[Assert\NotBlank] public string $email; + /** + * Optional external identifier used for matching existing subscribers during import. + */ + #[Assert\Length(max: 191)] + public ?string $foreignKey = null; + #[Assert\Type('bool')] public bool $confirmed; @@ -37,7 +43,8 @@ public function __construct( bool $htmlEmail, bool $disabled, ?string $extraData = null, - array $extraAttributes = [] + array $extraAttributes = [], + ?string $foreignKey = null, ) { $this->email = $email; $this->confirmed = $confirmed; @@ -47,5 +54,6 @@ public function __construct( $this->disabled = $disabled; $this->extraData = $extraData; $this->extraAttributes = $extraAttributes; + $this->foreignKey = $foreignKey; } } diff --git a/src/Domain/Subscription/Repository/SubscriberRepository.php b/src/Domain/Subscription/Repository/SubscriberRepository.php index 2ea02474..af776a75 100644 --- a/src/Domain/Subscription/Repository/SubscriberRepository.php +++ b/src/Domain/Subscription/Repository/SubscriberRepository.php @@ -16,6 +16,7 @@ * * @author Oliver Klee * @author Tatevik Grigoryan + * @SuppressWarnings(PHPMD.TooManyPublicMethods) */ class SubscriberRepository extends AbstractRepository implements PaginatableRepositoryInterface { @@ -41,6 +42,11 @@ public function findOneByUniqueId(string $uniqueId): ?Subscriber return $this->findOneBy(['uniqueId' => $uniqueId]); } + public function findOneByForeignKey(string $foreignKey): ?Subscriber + { + return $this->findOneBy(['foreignKey' => $foreignKey]); + } + public function findSubscribersBySubscribedList(int $listId): ?Subscriber { return $this->createQueryBuilder('s') diff --git a/src/Domain/Subscription/Service/CsvRowToDtoMapper.php b/src/Domain/Subscription/Service/CsvRowToDtoMapper.php index 606e58d8..0657c538 100644 --- a/src/Domain/Subscription/Service/CsvRowToDtoMapper.php +++ b/src/Domain/Subscription/Service/CsvRowToDtoMapper.php @@ -8,24 +8,45 @@ class CsvRowToDtoMapper { + private const FK_HEADER = 'foreignkey'; private const KNOWN_HEADERS = [ - 'email', 'confirmed', 'blacklisted', 'html_email', 'disabled', 'extra_data', + 'email', 'confirmed', 'blacklisted', 'html_email', 'disabled', 'extra_data', 'foreignkey', ]; public function map(array $row): ImportSubscriberDto { - $extraAttributes = array_filter($row, function ($key) { + // Normalize keys to lower-case for header matching safety (CSV library keeps original headers) + $normalizedRow = $this->normalizeData($row); + + $email = strtolower(trim((string)($normalizedRow['email'] ?? ''))); + + if (array_key_exists(self::FK_HEADER, $normalizedRow) && $normalizedRow[self::FK_HEADER] !== '') { + $foreignKey = (string)$normalizedRow[self::FK_HEADER]; + } + + $extraAttributes = array_filter($normalizedRow, function ($key) { return !in_array($key, self::KNOWN_HEADERS, true); }, ARRAY_FILTER_USE_KEY); return new ImportSubscriberDto( - email: trim($row['email'] ?? ''), - confirmed: filter_var($row['confirmed'] ?? false, FILTER_VALIDATE_BOOLEAN), - blacklisted: filter_var($row['blacklisted'] ?? false, FILTER_VALIDATE_BOOLEAN), - htmlEmail: filter_var($row['html_email'] ?? false, FILTER_VALIDATE_BOOLEAN), - disabled: filter_var($row['disabled'] ?? false, FILTER_VALIDATE_BOOLEAN), - extraData: $row['extra_data'] ?? null, - extraAttributes: $extraAttributes + email: $email, + confirmed: filter_var($normalizedRow['confirmed'] ?? false, FILTER_VALIDATE_BOOLEAN), + blacklisted: filter_var($normalizedRow['blacklisted'] ?? false, FILTER_VALIDATE_BOOLEAN), + htmlEmail: filter_var($normalizedRow['html_email'] ?? false, FILTER_VALIDATE_BOOLEAN), + disabled: filter_var($normalizedRow['disabled'] ?? false, FILTER_VALIDATE_BOOLEAN), + extraData: $normalizedRow['extra_data'] ?? null, + extraAttributes: $extraAttributes, + foreignKey: $foreignKey ?? null, ); } + + private function normalizeData(array $row): array + { + $normalizedRow = []; + foreach ($row as $key => $value) { + $normalizedRow[strtolower((string)$key)] = is_string($value) ? trim($value) : $value; + } + + return $normalizedRow; + } } diff --git a/src/Domain/Subscription/Service/Manager/SubscriberManager.php b/src/Domain/Subscription/Service/Manager/SubscriberManager.php index 1993cd9b..e0b7a3dd 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberManager.php @@ -114,6 +114,9 @@ public function createFromImport(ImportSubscriberDto $subscriberDto): Subscriber $subscriber->setHtmlEmail($subscriberDto->htmlEmail); $subscriber->setDisabled($subscriberDto->disabled); $subscriber->setExtraData($subscriberDto->extraData ?? ''); + if ($subscriberDto->foreignKey !== null) { + $subscriber->setForeignKey($subscriberDto->foreignKey); + } $this->entityManager->persist($subscriber); @@ -129,6 +132,9 @@ public function updateFromImport(Subscriber $existingSubscriber, ImportSubscribe $existingSubscriber->setHtmlEmail($subscriberDto->htmlEmail); $existingSubscriber->setDisabled($subscriberDto->disabled); $existingSubscriber->setExtraData($subscriberDto->extraData); + if ($subscriberDto->foreignKey !== null) { + $existingSubscriber->setForeignKey($subscriberDto->foreignKey); + } $uow = $this->entityManager->getUnitOfWork(); $meta = $this->entityManager->getClassMetadata(Subscriber::class); diff --git a/src/Domain/Subscription/Service/SubscriberCsvImporter.php b/src/Domain/Subscription/Service/SubscriberCsvImporter.php index e80db165..6873a03a 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvImporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvImporter.php @@ -182,7 +182,14 @@ private function processRow( return null; } - $subscriber = $this->subscriberRepository->findOneByEmail($dto->email); + [$subscriber, $conflictError] = $this->resolveSubscriber($dto); + + if ($conflictError !== null) { + $stats['skipped']++; + $stats['errors'][] = $conflictError; + return null; + } + if ($this->handleSkipCase($subscriber, $options, $stats)) { return null; } @@ -197,20 +204,7 @@ private function processRow( $this->attributeManager->processAttributes($subscriber, $dto->extraAttributes); - $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[] = $this->translator->trans( - 'Subscribed to %list%', - ['%list%' => $created->getSubscriberList()->getName()] - ); - } - } - } + [$listLines, $addedNewSubscriberToList] = $this->getHistoryListLines($subscriber, $options); if ($subscriber->isBlacklisted()) { $stats['blacklisted']++; @@ -226,6 +220,22 @@ private function processRow( return $this->prepareConfirmationMessage($subscriber, $options, $dto, $addedNewSubscriberToList); } + private function resolveSubscriber(ImportSubscriberDto $dto): array + { + $byEmail = $this->subscriberRepository->findOneByEmail($dto->email); + $byFk = null; + + if ($dto->foreignKey !== null) { + $byFk = $this->subscriberRepository->findOneByForeignKey($dto->foreignKey); + } + + if ($byEmail && $byFk && $byEmail->getId() !== $byFk->getId()) { + return [null, $this->translator->trans('Conflict: email and foreign key refer to different subscribers.')]; + } + + return [$byFk ?? $byEmail, null]; + } + private function handleInvalidEmail( ImportSubscriberDto $dto, SubscriberImportOptions $options, @@ -277,4 +287,27 @@ private function prepareConfirmationMessage( return null; } + + private function getHistoryListLines(Subscriber $subscriber, SubscriberImportOptions $options): array + { + $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[] = $this->translator->trans( + 'Subscribed to %list%', + ['%list%' => $created->getSubscriberList()->getName()] + ); + } + } + } + + return [ + $listLines, + $addedNewSubscriberToList, + ]; + } } diff --git a/src/Migrations/Version20251103SeedInitialAdmin.php b/src/Migrations/Version20251103SeedInitialAdmin.php new file mode 100644 index 00000000..c1d95889 --- /dev/null +++ b/src/Migrations/Version20251103SeedInitialAdmin.php @@ -0,0 +1,49 @@ +connection->getDatabasePlatform(); + $this->skipIf(!$platform instanceof PostgreSQLPlatform, sprintf( + 'Unsupported platform for this migration: %s', + get_class($platform) + )); + + $this->addSql(<<<'SQL' + INSERT INTO phplist_admin (id, created, modified, loginname, namelc, email, password, passwordchanged, disabled, superuser, privileges) + VALUES (1, NOW(), NOW(), 'admin', 'admin', 'admin@example.com', :hash, CURRENT_DATE, FALSE, TRUE, :privileges) + ON CONFLICT (id) DO UPDATE + SET + modified = EXCLUDED.modified, + privileges = EXCLUDED.privileges + SQL, [ + 'hash' => hash('sha256', 'password'), + 'privileges' => 'a:4:{s:11:"subscribers";b:1;s:9:"campaigns";b:1;s:10:"statistics";b:1;s:8:"settings";b:1;}', + ]); + } + + public function down(Schema $schema): void + { + $platform = $this->connection->getDatabasePlatform(); + $this->skipIf(!$platform instanceof PostgreSQLPlatform, sprintf( + 'Unsupported platform for this migration: %s', + get_class($platform) + )); + + $this->addSql('DELETE FROM phplist_admin WHERE id = 1'); + } +} diff --git a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php index 424a7e0d..884bcc7e 100644 --- a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php +++ b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php @@ -158,6 +158,10 @@ public function testImportFromCsvUpdatesExistingSubscribers(): void ->with('existing@example.com') ->willReturn($existingSubscriber); + $this->subscriberRepositoryMock + ->method('findOneByForeignKey') + ->willReturn(null); + $importDto = new ImportSubscriberDto( email: 'existing@example.com', confirmed: true, @@ -190,6 +194,199 @@ public function testImportFromCsvUpdatesExistingSubscribers(): void ] )); + $this->attributeManagerMock + ->expects($this->once()) + ->method('processAttributes') + ->with($existingSubscriber); + + $options = new SubscriberImportOptions(updateExisting: true); + $result = $this->subject->importFromCsv($uploadedFile, $options); + + $this->assertSame(0, $result['created']); + $this->assertSame(1, $result['updated']); + $this->assertSame(0, $result['skipped']); + $this->assertEmpty($result['errors']); + + unlink($tempFile); + } + + public function testImportResolvesByForeignKeyWhenProvidedAndMatches(): void + { + $tempFile = tempnam(sys_get_temp_dir(), 'csv_test_fk'); + file_put_contents($tempFile, "email,confirmed,html_email,blacklisted,disabled,foreignkey\n" . + "user@example.com,1,1,0,0,EXT-123\n"); + + $uploadedFile = $this->createMock(UploadedFile::class); + $uploadedFile->method('getRealPath')->willReturn($tempFile); + + $existingByFk = $this->createMock(Subscriber::class); + $existingByFk->method('getId')->willReturn(10); + + $this->subscriberRepositoryMock + ->method('findOneByEmail') + ->with('user@example.com') + ->willReturn(null); + + $this->subscriberRepositoryMock + ->method('findOneByForeignKey') + ->with('EXT-123') + ->willReturn($existingByFk); + + $dto = new ImportSubscriberDto( + email: 'user@example.com', + confirmed: true, + blacklisted: false, + htmlEmail: true, + disabled: false, + extraData: null, + extraAttributes: [], + foreignKey: 'EXT-123', + ); + + $this->csvImporterMock + ->method('import') + ->with($tempFile) + ->willReturn([ + 'valid' => [$dto], + 'errors' => [] + ]); + + $this->subscriberManagerMock + ->expects($this->once()) + ->method('updateFromImport') + ->with($existingByFk, $dto) + ->willReturn(new ChangeSetDto()); + + $this->attributeManagerMock + ->expects($this->once()) + ->method('processAttributes') + ->with($existingByFk); + + $options = new SubscriberImportOptions(updateExisting: true); + $result = $this->subject->importFromCsv($uploadedFile, $options); + + $this->assertSame(0, $result['created']); + $this->assertSame(1, $result['updated']); + $this->assertSame(0, $result['skipped']); + $this->assertEmpty($result['errors']); + + unlink($tempFile); + } + + public function testImportConflictWhenEmailAndForeignKeyReferToDifferentSubscribers(): void + { + $tempFile = tempnam(sys_get_temp_dir(), 'csv_test_fk_conflict'); + file_put_contents($tempFile, "email,confirmed,html_email,blacklisted,disabled,foreignkey\n" . + "conflict@example.com,1,1,0,0,EXT-999\n"); + + $uploadedFile = $this->createMock(UploadedFile::class); + $uploadedFile->method('getRealPath')->willReturn($tempFile); + + $byEmail = $this->createMock(Subscriber::class); + $byEmail->method('getId')->willReturn(1); + $byFk = $this->createMock(Subscriber::class); + $byFk->method('getId')->willReturn(2); + + $this->subscriberRepositoryMock + ->method('findOneByEmail') + ->with('conflict@example.com') + ->willReturn($byEmail); + + $this->subscriberRepositoryMock + ->method('findOneByForeignKey') + ->with('EXT-999') + ->willReturn($byFk); + + $dto = new ImportSubscriberDto( + email: 'conflict@example.com', + confirmed: true, + blacklisted: false, + htmlEmail: true, + disabled: false, + extraData: null, + extraAttributes: [], + foreignKey: 'EXT-999', + ); + + $this->csvImporterMock + ->method('import') + ->with($tempFile) + ->willReturn([ + 'valid' => [$dto], + 'errors' => [] + ]); + + $this->subscriberManagerMock + ->expects($this->never()) + ->method('createFromImport'); + $this->subscriberManagerMock + ->expects($this->never()) + ->method('updateFromImport'); + + $options = new SubscriberImportOptions(updateExisting: true); + $result = $this->subject->importFromCsv($uploadedFile, $options); + + $this->assertSame(0, $result['created']); + $this->assertSame(0, $result['updated']); + $this->assertSame(1, $result['skipped']); + $this->assertCount(1, $result['errors']); + $this->assertSame('Conflict: email and foreign key refer to different subscribers.', $result['errors'][0]); + + unlink($tempFile); + } + + public function testImportResolvesByEmailWhenForeignKeyNotFound(): void + { + $tempFile = tempnam(sys_get_temp_dir(), 'csv_test_fk_email'); + file_put_contents($tempFile, "email,confirmed,html_email,blacklisted,disabled,foreignkey\n" . + "existing@example.com,1,1,0,0,EXT-404\n"); + + $uploadedFile = $this->createMock(UploadedFile::class); + $uploadedFile->method('getRealPath')->willReturn($tempFile); + + $existingByEmail = $this->createMock(Subscriber::class); + $existingByEmail->method('getId')->willReturn(5); + + $this->subscriberRepositoryMock + ->method('findOneByEmail') + ->with('existing@example.com') + ->willReturn($existingByEmail); + + $this->subscriberRepositoryMock + ->method('findOneByForeignKey') + ->with('EXT-404') + ->willReturn(null); + + $dto = new ImportSubscriberDto( + email: 'existing@example.com', + confirmed: true, + blacklisted: false, + htmlEmail: true, + disabled: false, + extraData: null, + extraAttributes: [], + foreignKey: 'EXT-404', + ); + + $this->csvImporterMock + ->method('import') + ->with($tempFile) + ->willReturn([ + 'valid' => [$dto], + 'errors' => [] + ]); + + $this->subscriberManagerMock + ->expects($this->once()) + ->method('updateFromImport') + ->with($existingByEmail, $dto) + ->willReturn(new ChangeSetDto()); + + $this->attributeManagerMock + ->expects($this->once()) + ->method('processAttributes') + ->with($existingByEmail); + $options = new SubscriberImportOptions(updateExisting: true); $result = $this->subject->importFromCsv($uploadedFile, $options); @@ -200,4 +397,67 @@ public function testImportFromCsvUpdatesExistingSubscribers(): void unlink($tempFile); } + + public function testImportCreatesNewWhenNeitherEmailNorForeignKeyFound(): void + { + $tempFile = tempnam(sys_get_temp_dir(), 'csv_test_fk_create'); + file_put_contents($tempFile, "email,confirmed,html_email,blacklisted,disabled,foreignkey\n" . + "new@example.com,0,1,0,0,NEW-KEY\n"); + + $uploadedFile = $this->createMock(UploadedFile::class); + $uploadedFile->method('getRealPath')->willReturn($tempFile); + + $this->subscriberRepositoryMock + ->method('findOneByEmail') + ->with('new@example.com') + ->willReturn(null); + + $this->subscriberRepositoryMock + ->method('findOneByForeignKey') + ->with('NEW-KEY') + ->willReturn(null); + + $dto = new ImportSubscriberDto( + email: 'new@example.com', + confirmed: false, + blacklisted: false, + htmlEmail: true, + disabled: false, + extraData: null, + extraAttributes: [], + foreignKey: 'NEW-KEY', + ); + + $this->csvImporterMock + ->method('import') + ->with($tempFile) + ->willReturn([ + 'valid' => [$dto], + 'errors' => [] + ]); + + $created = $this->createMock(Subscriber::class); + $created->method('getId')->willReturn(100); + + $this->subscriberManagerMock + ->expects($this->once()) + ->method('createFromImport') + ->with($dto) + ->willReturn($created); + + $this->attributeManagerMock + ->expects($this->once()) + ->method('processAttributes') + ->with($created); + + $options = new SubscriberImportOptions(updateExisting: false); + $result = $this->subject->importFromCsv($uploadedFile, $options); + + $this->assertSame(1, $result['created']); + $this->assertSame(0, $result['updated']); + $this->assertSame(0, $result['skipped']); + $this->assertEmpty($result['errors']); + + unlink($tempFile); + } }