Import by foreign key

[TatevikGr]

Summary

• Import by foreign key

Unit test

Are your changes covered with unit tests, and do they not break anything?

You can run the existing unit tests using this command:

vendor/bin/phpunit tests/

Code style

Have you checked that you code is well-documented and follows the PSR-2 coding
style?

You can check for this using this command:

vendor/bin/phpcs --standard=PSR2 src/ tests/

Other Information

If there's anything else that's important and relevant to your pull
request, mention that information here. This could include benchmarks,
or other information.

If you are updating any of the CHANGELOG files or are asked to update the
CHANGELOG files by reviewers, please add the CHANGELOG entry at the top of the file.

Thanks for contributing to phpList!

Summary by CodeRabbit

• New Features
  • CSV subscriber imports now support an optional foreign key field for identifying subscribers.
  • Added conflict detection: when a foreign key and email refer to different subscribers, the import row is skipped and logged as an error.

[coderabbitai]

Walkthrough

The changes introduce foreign key support to the subscriber import system. A foreignKey field is added to the data transfer object, repository queries, and CSV mapper. The importer is enhanced to resolve subscribers by foreign key and detect conflicts when email and foreign key refer to different subscribers.

Changes

Cohort / File(s)	Summary
Translation & Configuration resources/translations/messages.en.xlf	Added new translation unit for conflict detection message when email and foreign key refer to different subscribers.
Data Model Enhancement src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php	Added public foreignKey?: string property with max length constraint of 191 characters; updated constructor to accept optional foreignKey parameter.
Repository & Persistence src/Domain/Subscription/Repository/SubscriberRepository.php, src/Domain/Subscription/Service/Manager/SubscriberManager.php	Added findOneByForeignKey() method to SubscriberRepository; updated SubscriberManager to conditionally set foreignKey on Subscriber during createFromImport and updateFromImport when provided.
CSV Processing & Mapping src/Domain/Subscription/Service/CsvRowToDtoMapper.php	Introduced normalizeData() method to lowercase keys and trim string values; added FK_HEADER constant and extended KNOWN_HEADERS; refactored map() to normalize input row and extract optional foreignKey field; updated ImportSubscriberDto instantiation to pass foreignKey.
Import Conflict Detection src/Domain/Subscription/Service/SubscriberCsvImporter.php	Introduced resolveSubscriber() method to check both email and foreign key, detecting conflicts when they refer to different subscribers; replaced direct email lookup with conflict-aware resolution; introduced getHistoryListLines() helper to encapsulate list-subscription line generation logic.
Test Coverage tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php	Added comprehensive test cases for foreign-key resolution, conflict detection, and creation paths; tests verify update/create flows with attribute processing across scenarios where email matches, foreign key matches, conflicts occur, or neither exist.

Sequence Diagram

sequenceDiagram
    participant Importer as SubscriberCsvImporter
    participant Repo as SubscriberRepository
    participant Mgr as SubscriberManager
    participant Sub as Subscriber

    Importer->>Importer: resolveSubscriber(dto)
    Importer->>Repo: findOneByEmail(email)
    Repo-->>Importer: subscriber1 or null
    
    alt foreignKey provided
        Importer->>Repo: findOneByForeignKey(foreignKey)
        Repo-->>Importer: subscriber2 or null
        
        alt subscriber1 and subscriber2 differ
            Importer->>Importer: Conflict detected!
            Importer->>Importer: Skip row, record error
        else subscriber1 exists and matches or subscriber2 exists
            Importer->>Mgr: updateFromImport(dto)
            Mgr->>Sub: set foreignKey
        else neither exists
            Importer->>Mgr: createFromImport(dto)
            Mgr->>Sub: new Subscriber + foreignKey
        end
    else no foreignKey
        alt subscriber1 exists
            Importer->>Mgr: updateFromImport(dto)
        else
            Importer->>Mgr: createFromImport(dto)
        end
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• CsvRowToDtoMapper.php: Refactoring introduces new normalization logic and data flow; requires careful verification that all field extraction paths are correct and that attribute computation excludes proper headers.
• SubscriberCsvImporter.php: Conflict detection logic and the split of responsibilities between resolveSubscriber() and getHistoryListLines() need thorough review to ensure edge cases are handled.
• Test coverage: Comprehensive new tests provide good coverage; verify test assertions align with expected behavior and that duplicate test cases (if any) are intentional.

Poem

🐰 A foreign key arrives with grace,
Matching subscribers, face to face!
When conflicts arise, we skip with care,
Our CSV import, now conflict-aware! 🌾✨

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Import by foreign key' clearly and directly summarizes the main change across all modified files, which collectively implement foreign key support for subscriber imports.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch import-by-foreign-key

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

src/Domain/Subscription/Service/SubscriberCsvImporter.php (2)

223-237: LGTM: Solid conflict detection logic.

The resolveSubscriber method correctly handles all scenarios:

• Matches by email or foreign key independently
• Detects conflicts when both identifiers point to different subscribers
• Prioritizes foreign key matches (line 236), which makes sense as foreign keys are typically more specific identifiers

The logic correctly prevents data corruption by rejecting ambiguous imports.

Optional: Consider a more type-safe return value.

The method currently returns a plain array [?Subscriber, ?string]. For improved type safety and clarity, consider introducing a simple result class:

final class SubscriberResolutionResult
{
    public function __construct(
        public readonly ?Subscriber $subscriber,
        public readonly ?string $error
    ) {}
}

Then update the method signature and return statement:

-    private function resolveSubscriber(ImportSubscriberDto $dto): array
+    private function resolveSubscriber(ImportSubscriberDto $dto): SubscriberResolutionResult
     {
         // ... existing logic ...
         
         if ($byEmail && $byFk && $byEmail->getId() !== $byFk->getId()) {
-            return [null, $this->translator->trans('Conflict: email and foreign key refer to different subscribers.')];
+            return new SubscriberResolutionResult(null, $this->translator->trans('Conflict: email and foreign key refer to different subscribers.'));
         }
 
-        return [$byFk ?? $byEmail, null];
+        return new SubscriberResolutionResult($byFk ?? $byEmail, null);
     }

And update the caller at line 185:

-        [$subscriber, $conflictError] = $this->resolveSubscriber($dto);
+        $result = $this->resolveSubscriber($dto);
+        $subscriber = $result->subscriber;
+        $conflictError = $result->error;

This makes the intent explicit and enables IDE autocompletion.

---

207-207: LGTM: Excellent refactoring of list subscription logic.

The extraction of getHistoryListLines improves code organization by:

• Reducing complexity in processRow
• Encapsulating list subscription and history message generation in a single, focused method
• Returning both the history messages and a flag for downstream confirmation logic

The logic correctly handles blacklisted subscribers and builds appropriate history entries for each successful list subscription.

Optional: Similar type safety consideration as resolveSubscriber.

The method returns a plain array [array, bool]. For consistency with a potential SubscriberResolutionResult, you could introduce a simple value object:

final class HistoryListResult
{
    public function __construct(
        public readonly array $listLines,
        public readonly bool $addedNewSubscriberToList
    ) {}
}

This is a minor improvement and can be deferred.

Also applies to: 291-312

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 938b090 and 7461691.

📒 Files selected for processing (7)

• resources/translations/messages.en.xlf (1 hunks)
• src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php (3 hunks)
• src/Domain/Subscription/Repository/SubscriberRepository.php (2 hunks)
• src/Domain/Subscription/Service/CsvRowToDtoMapper.php (1 hunks)
• src/Domain/Subscription/Service/Manager/SubscriberManager.php (2 hunks)
• src/Domain/Subscription/Service/SubscriberCsvImporter.php (4 hunks)
• src/Migrations/Version20251103SeedInitialAdmin.php (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

src/Domain/Subscription/Service/CsvRowToDtoMapper.php (1)

src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php (1)

• ImportSubscriberDto (9-59)

src/Domain/Subscription/Service/Manager/SubscriberManager.php (1)

src/Domain/Subscription/Model/Subscriber.php (1)

• setForeignKey (374-377)

src/Domain/Subscription/Service/SubscriberCsvImporter.php (6)

src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php (1)

• ImportSubscriberDto (9-59)

src/Domain/Subscription/Repository/SubscriberRepository.php (2)

• findOneByEmail (35-38)
• findOneByForeignKey (45-48)

src/Domain/Subscription/Model/Dto/SubscriberImportOptions.php (1)

• SubscriberImportOptions (7-19)

src/Domain/Subscription/Model/Subscriber.php (1)

• isBlacklisted (154-157)

src/Domain/Subscription/Service/Manager/SubscriptionManager.php (1)

• addSubscriberToAList (39-59)

src/Domain/Subscription/Model/Subscription.php (1)

• getSubscriberList (75-78)

🔇 Additional comments (8)

resources/translations/messages.en.xlf (1)

733-736: Conflict string captured correctly.

The new translation mirrors the source and keeps the __ prefix consistent with neighboring entries. Looks good.

src/Domain/Subscription/Service/Manager/SubscriberManager.php (1)

117-119: Good call on preserving optional foreign keys.

Conditionally syncing the foreign key only when present keeps imports idempotent and avoids clobbering existing values unintentionally. 👍

Also applies to: 135-137

src/Domain/Subscription/Repository/SubscriberRepository.php (1)

45-48: Repository lookup addition looks solid.

The dedicated findOneByForeignKey() keeps the import resolver simple and reuses Doctrine’s query building safely. No concerns here.

src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php (1)

14-58: DTO update aligns with the new import flow.

Adding the nullable foreignKey property with validation and extending the constructor keeps existing callers working while exposing the new field. Looks great.

src/Domain/Subscription/Service/CsvRowToDtoMapper.php (3)

11-14: LGTM: Clean constant definitions for foreign key support.

The FK_HEADER constant and extension of KNOWN_HEADERS follow the existing pattern and clearly define the expected CSV header for foreign key matching.

---

18-40: LGTM: Robust normalization and foreign key extraction.

The normalization flow centralizes key case-insensitivity and value trimming, making the CSV parsing more resilient to header variations. The foreign key extraction correctly handles optional values and prevents empty strings from being treated as valid foreign keys.

---

43-51: LGTM: Well-implemented normalization helper.

The normalizeData method provides clean key normalization (case-insensitive matching) and selective value trimming (strings only), which correctly handles mixed-type CSV data without altering non-string values.

src/Domain/Subscription/Service/SubscriberCsvImporter.php (1)

185-191: LGTM: Clean conflict detection integration.

The conflict handling correctly increments the skipped counter, records the error message, and returns early to prevent processing conflicting data.

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7461691 and e08058d.

📒 Files selected for processing (2)

• config/parameters.yml.dist (1 hunks)
• src/Migrations/Version20251103SeedInitialAdmin.php (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: phpList on PHP 8.1 [Build, Test]
• GitHub Check: phpList on PHP 8.1 [Build, Test]

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e08058d and aa4b938.

📒 Files selected for processing (2)

• config/parameters.yml.dist (1 hunks)
• src/Migrations/Version20251103SeedInitialAdmin.php (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• config/parameters.yml.dist

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: phpList on PHP 8.1 [Build, Test]
• GitHub Check: phpList on PHP 8.1 [Build, Test]

🔇 Additional comments (2)

src/Migrations/Version20251103SeedInitialAdmin.php (2)

35-43: LGTM! Password validation addresses the previous critical issue.

The password resolution logic correctly prioritizes the container parameter, falls back to the environment variable, and validates that the resolved password is not empty or whitespace-only before proceeding. This addresses the previously flagged critical issue.

---

66-66: LGTM! Safe rollback logic.

The DELETE statement is now scoped to only remove the admin user created by this migration (matching id, loginname, and email). This addresses the previous critical issue and ensures pre-existing admins with id = 1 are not inadvertently removed during rollback.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

src/Domain/Identity/Command/ImportDefaultsCommand.php (1)

45-68: Consider error handling for database operations.

The command does not handle potential exceptions from database operations (e.g., createAdministrator, flush) or constraint violations if an administrator with the same email but different login already exists.

Consider wrapping the database operations in a try-catch block:

try {
    $existing = $this->administratorRepository->findOneBy(['loginName' => $login]);
    if ($existing === null) {
        // ... creation logic
        $this->entityManager->flush();
        $output->writeln(/* success message */);
    } else {
        $output->writeln(/* already exists message */);
    }
} catch (\Exception $e) {
    $output->writeln('<error>Failed to import defaults: ' . $e->getMessage() . '</error>');
    return Command::FAILURE;
}

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa4b938 and 333eade.

📒 Files selected for processing (3)

• .coderabbit.yaml (1 hunks)
• .github/PULL_REQUEST_TEMPLATE.md (1 hunks)
• src/Domain/Identity/Command/ImportDefaultsCommand.php (1 hunks)

✅ Files skipped from review due to trivial changes (1)

• .github/PULL_REQUEST_TEMPLATE.md

🧰 Additional context used

🧬 Code graph analysis (1)

src/Domain/Identity/Command/ImportDefaultsCommand.php (3)

src/Domain/Identity/Model/Dto/CreateAdministratorDto.php (1)

• CreateAdministratorDto (7-20)

src/Domain/Identity/Service/AdministratorManager.php (2)

• AdministratorManager (13-61)
• createAdministrator (24-37)

src/Domain/Identity/Model/Administrator.php (2)

• isSuperUser (151-154)
• getLoginName (90-93)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: phpList on PHP 8.1 [Build, Test]
• GitHub Check: phpList on PHP 8.1 [Build, Test]

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

src/Domain/Identity/Command/ImportDefaultsCommand.php (1)

79-82: Report the actual stored email for existing admin.

This branch still prints the hardcoded $email constant instead of $existing->getEmail(), so the message is wrong whenever the administrator’s email differs from the default. Please use the real value.

Apply this diff:

             $output->writeln(sprintf(
                 'Default admin already exists: login="%s", email="%s"',
                 $existing->getLoginName(),
-                $email,
+                $existing->getEmail(),
             ));

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 333eade and c333bdb.

📒 Files selected for processing (1)

• src/Domain/Identity/Command/ImportDefaultsCommand.php (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/Domain/Identity/Command/ImportDefaultsCommand.php (3)

src/Domain/Identity/Model/Dto/CreateAdministratorDto.php (1)

• CreateAdministratorDto (7-20)

src/Domain/Identity/Service/AdministratorManager.php (2)

• AdministratorManager (13-61)
• createAdministrator (24-37)

src/Domain/Identity/Model/Administrator.php (2)

• isSuperUser (151-154)
• getLoginName (90-93)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: phpList on PHP 8.1 [Build, Test]
• GitHub Check: phpList on PHP 8.1 [Build, Test]

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

resources/translations/messages.en.xlf (1)

733-736: Verify the "__" prefix pattern is intentional.

The new translation entry follows the pattern seen in recent additions (from line 393 onwards) where target text is prefixed with "__". However, earlier entries (lines 8-365) have matching source and target without this prefix.

Please confirm:

• Is the "__" prefix intentional for this en->en translation file?
• Does it indicate a placeholder or pending review status?

Optional enhancement: Consider adding placeholder variables for the email and foreign key values to make error messages more actionable for debugging:

-      <trans-unit id="TBYUW2m" resname="Conflict: email and foreign key refer to different subscribers.">
-        <source>Conflict: email and foreign key refer to different subscribers.</source>
-        <target>__Conflict: email and foreign key refer to different subscribers.</target>
+      <trans-unit id="TBYUW2m" resname="Conflict: email '%email%' and foreign key '%foreign_key%' refer to different subscribers.">
+        <source>Conflict: email '%email%' and foreign key '%foreign_key%' refer to different subscribers.</source>
+        <target>__Conflict: email '%email%' and foreign key '%foreign_key%' refer to different subscribers.</target>
       </trans-unit>

This would provide more context when the error occurs, similar to other messages in the file (e.g., lines 137-139, 377-379).

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c3d4ef2 and 3ee2fdf.

📒 Files selected for processing (4)

• resources/translations/messages.en.xlf (1 hunks)
• src/Domain/Subscription/Service/CsvRowToDtoMapper.php (1 hunks)
• src/Domain/Subscription/Service/SubscriberCsvImporter.php (4 hunks)
• tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php (3)

src/Domain/Subscription/Model/Dto/SubscriberImportOptions.php (1)

• SubscriberImportOptions (7-19)

src/Domain/Subscription/Service/SubscriberCsvImporter.php (1)

• importFromCsv (71-132)

src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php (1)

• ImportSubscriberDto (9-59)

src/Domain/Subscription/Service/CsvRowToDtoMapper.php (1)

src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php (1)

• ImportSubscriberDto (9-59)

src/Domain/Subscription/Service/SubscriberCsvImporter.php (6)

src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php (1)

• ImportSubscriberDto (9-59)

src/Domain/Subscription/Repository/SubscriberRepository.php (2)

• findOneByEmail (35-38)
• findOneByForeignKey (45-48)

src/Domain/Subscription/Model/Dto/SubscriberImportOptions.php (1)

• SubscriberImportOptions (7-19)

src/Domain/Subscription/Model/Subscriber.php (1)

• isBlacklisted (154-157)

src/Domain/Subscription/Service/Manager/SubscriptionManager.php (1)

• addSubscriberToAList (39-59)

src/Domain/Subscription/Model/Subscription.php (1)

• getSubscriberList (75-78)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: phpList on PHP 8.1 [Build, Test]
• GitHub Check: phpList on PHP 8.1 [Build, Test]

🔇 Additional comments (2)

tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php (1)

276-334: Conflict flow test is well targeted.

Appreciate the explicit coverage of the mismatched email/foreign key path and the assertions guarding against accidental updates or creations. This gives confidence the new conflict handling won’t regress silently.

src/Domain/Subscription/Service/CsvRowToDtoMapper.php (1)

43-51: Nice normalization helper.
Centralizing the lower-casing/trim step up front removes the header-case brittleness we were hitting and makes the mapper much safer.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Expose conflicting identifiers in the error.
Right now the translator string we bubble up is just “Conflict: email and foreign key refer to different subscribers.” so operators never learn which email/foreign key pair triggered the skip. Without that context the CSV owner has no actionable clue which row to fix, which is a regression in usability compared to the other error paths where we embed %email%. Please include the DTO context in the message before returning.

Apply this diff to preserve the conflict check while surfacing the identifiers:

-        if ($byEmail && $byFk && $byEmail->getId() !== $byFk->getId()) {
-            return [null, $this->translator->trans('Conflict: email and foreign key refer to different subscribers.')];
+        if ($byEmail && $byFk && $byEmail->getId() !== $byFk->getId()) {
+            return [
+                null,
+                $this->translator->trans(
+                    'Conflict: email "%email%" and foreign key "%foreignKey%" refer to different subscribers.',
+                    [
+                        '%email%' => $dto->email,
+                        '%foreignKey%' => $dto->foreignKey,
+                    ],
+                ),
+            ];
         }

🤖 Prompt for AI Agents

In src/Domain/Subscription/Service/SubscriberCsvImporter.php around lines
232-234, the conflict message omits the specific email and foreign key so
operators can't identify the problematic row; keep the existing identity check
but change the returned translated string to include the DTO context by calling
$this->translator->trans with placeholders (e.g. %email% and %foreignKey%) and
pass the DTO values (email and foreign key) as parameters so the error becomes
something like "Conflict: email %email% and foreign key %foreignKey% refer to
different subscribers." before returning.