fix(review): #1269 review findings, round 2

docs/features/cmdb-import.md

@@ -377,7 +377,12 @@ placeholder names only: once you rename the columns, an export that has
 them on only one of the two CMDB sheets warns that the column is missing
 on the other. Once you add the employee number, an owner who was found by
 name or e-mail address until then gets a new contact; the old contact
-stays in **Stackiq CMDB owners**.
+stays in **Stackiq CMDB owners**. The same happens when an owner first
+imported by name alone arrives with an e-mail address: the name-only
+contact is not found by that address, so a second contact is created next
+to it. An instance without a `secret` in `config.php` cannot use the
+employee number: owners are then matched on e-mail address or name, so
+two namesakes become one contact (the log warns once).
 
 When the Contacts app is disabled, applications and usages are still
 imported; the owners are skipped and each affected row carries a warning.

lib/Service/Cmdb/CmdbImportProfile.php

@@ -1163,8 +1163,8 @@ private function resolveValidator(): object {
 		}
 
 		if (class_exists($class) === true) {
-			$validator = new $class();
-			if (method_exists($validator, 'validate') === true) {
+			$validator = (new OpenRegisterInstance(container: $this->container))->build(class: $class);
+			if ($validator !== null && method_exists($validator, 'validate') === true) {
 				return $validator;
 			}
 		}

lib/Service/Cmdb/CmdbOwnerIdentity.php

@@ -74,8 +74,10 @@ public function __construct(
 	 * e-mail address or the name instead of sharing one UID with every
 	 * other owner whose cell holds the same placeholder.
 	 *
-	 * Fail-mode (ADR-102): closed. Without a `secret` the number is not used
-	 * (null, logged once), never hashed with an empty key. A changed secret
+	 * Fail-mode (ADR-102): degrades. Without a `secret` the employee number is
+	 * not used (null, logged once), never hashed with an empty key, and the
+	 * owner is matched on the e-mail address or the name like before the
+	 * number existed, so two namesakes become one contact. A changed secret
 	 * gives every owner a new UID, so a new contact on the next import.
 	 *
 	 * @param string $employeeNumber The employee number from the owner pack.

lib/Service/Cmdb/OpenRegisterInstance.php

@@ -0,0 +1,94 @@
+<?php
+
+/**
+ * Constructs OpenRegister's pack validator and mapping engine across versions.
+ *
+ * Newer OpenRegister versions give these classes a required `IL10N`
+ * constructor parameter, older ones have none. Constructing them by reflection
+ * works against both.
+ *
+ * @category  Service
+ * @package   OCA\Stackiq\Service\Cmdb
+ * @author    Conduction b.v. <[email protected]>
+ * @copyright 2026 Conduction B.V.
+ * @license   EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12
+ * @link      https://github.com/ConductionNL/stackiq
+ *
+ * @spec openspec/changes/cmdb-export-import/tasks.md#task-3
+ *
+ * SPDX-FileCopyrightText: 2026 Conduction B.V. <[email protected]>
+ * SPDX-License-Identifier: EUPL-1.2
+ */
+
+declare(strict_types=1);
+
+namespace OCA\Stackiq\Service\Cmdb;
+
+use OCP\IL10N;
+use OCP\L10N\IFactory;
+use Psr\Container\ContainerInterface;
+use ReflectionClass;
+use Throwable;
+
+/**
+ * Builds an OpenRegister class with the constructor arguments it declares.
+ *
+ * @spec openspec/changes/cmdb-export-import/tasks.md#task-3
+ */
+class OpenRegisterInstance {
+	/**
+	 * Constructor.
+	 *
+	 * @param ContainerInterface $container The container that resolves the translation factory.
+	 */
+	public function __construct(
+		private readonly ContainerInterface $container,
+	) {
+	}//end __construct()
+
+	/**
+	 * Construct the class: without arguments, or with OpenRegister's translations when its constructor takes an IL10N.
+	 *
+	 * @param string $class The OpenRegister class.
+	 *
+	 * @return object|null The instance, or null when it cannot be built.
+	 *
+	 * @spec openspec/changes/cmdb-export-import/tasks.md#task-3
+	 */
+	public function build(string $class): ?object {
+		try {
+			$constructor = (new ReflectionClass($class))->getConstructor();
+			$arguments = [];
+			foreach (($constructor?->getParameters() ?? []) as $parameter) {
+				if ($parameter->isOptional() === true) {
+					break;
+				}
+
+				$type = $parameter->getType();
+				if ($type instanceof \ReflectionNamedType && $type->getName() === IL10N::class) {
+					$arguments[] = $this->l10n();
+					continue;
+				}
+
+				return null;
+			}
+
+			return new $class(...$arguments);
+		} catch (Throwable $e) {
+			return null;
+		}
+	}//end build()
+
+	/**
+	 * OpenRegister's translations.
+	 *
+	 * @return IL10N
+	 *
+	 * @spec openspec/changes/cmdb-export-import/tasks.md#task-3
+	 */
+	private function l10n(): IL10N {
+		$factory = $this->container->get(IFactory::class);
+
+		return $factory->get('openregister');
+	}//end l10n()
+}//end class

lib/Service/CmdbExportImportService.php

@@ -69,6 +69,7 @@
 use OCA\Stackiq\Service\Cmdb\CmdbImportProfile;
 use OCA\Stackiq\Service\Cmdb\CmdbImportReport;
 use OCA\Stackiq\Service\Cmdb\CmdbOwnerIdentity;
+use OCA\Stackiq\Service\Cmdb\OpenRegisterInstance;
 use OCA\Stackiq\Service\Cmdb\CmdbRowNormaliser;
 use OCA\Stackiq\Service\Cmdb\CmdbWorkbookReader;
 use OCP\AppFramework\Db\DoesNotExistException;
@@ -1653,6 +1654,14 @@ private function existingOwnerContact(?string $numberUid, string $email, string
 	/**
 	 * The contact in the owners' address book with exactly this UID.
 	 *
+	 * Nextcloud indexes the UID property of every card (CardDavBackend
+	 * INDEXED_PROPERTIES) and AddressBookImpl::search() matches it with a
+	 * LIKE on that index, so a search on ['UID'] finds the card. Should the
+	 * search miss it anyway, StackiqContactSyncService::syncToNamedAddressBook()
+	 * reuses a card found by getCard('<uid>.vcf') in the same address book. A
+	 * card with this UID at a different URI is not found that way: createCard
+	 * then throws UidConflict, which is caught and returns null.
+	 *
 	 * @param string $uid The contact UID.
 	 *
 	 * @return string|null The contact UID, or null.
@@ -2911,8 +2920,8 @@ private function resolveEngine(): object {
 		}
 
 		if (class_exists($class) === true) {
-			$engine = new $class();
-			if (method_exists($engine, 'mapRow') === true) {
+			$engine = (new OpenRegisterInstance(container: $this->container))->build(class: $class);
+			if ($engine !== null && method_exists($engine, 'mapRow') === true) {
 				return $engine;
 			}
 		}

lib/Service/StackiqContactSyncService.php

@@ -290,8 +290,10 @@ public function searchNamedAddressBook(string $query, string $addressBookUri, ar
 	/**
 	 * The contact in the named address book with the record's e-mail address, or null.
 	 *
-	 * Always null with a fixed UID: the caller identifies the contact by
-	 * that UID, and a shared e-mail address must not merge two of them.
+	 * With a fixed UID the e-mail address matches nothing: the caller
+	 * identifies the contact by that UID, and a shared e-mail address must
+	 * not merge two of them. Only a card that already has that UID (by URI)
+	 * is returned.
 	 *
 	 * @param array<string, mixed> $record The relationship record.
 	 * @param string $addressBookUri The address book's URI.
@@ -303,6 +305,19 @@ public function searchNamedAddressBook(string $query, string $addressBookUri, ar
 	 */
 	private function namedAddressBookContactByEmail(array $record, string $addressBookUri, ?string $fixedUid): ?string {
 		if ($fixedUid !== null) {
+			// A card that already has the fixed UID is the contact: Nextcloud
+			// refuses a second one (UidConflict), and the contacts search may not
+			// show it yet. Looked up by URI, not by search.
+			$key = $this->existingNamedAddressBookKey(addressBookUri: $addressBookUri);
+			$backend = null;
+			if ($key !== null) {
+				$backend = $this->container?->get(static::CARDDAV_BACKEND_CLASS);
+			}
+
+			if (is_object($backend) === true && is_array($backend->getCard((int)$key, $fixedUid . '.vcf')) === true) {
+				return $fixedUid;
+			}
+
 			return null;
 		}
 

tests/Unit/Controller/SettingsControllerCmdbImportMappingTest.php

@@ -89,9 +89,7 @@ protected function tearDown(): void {
 	 * @return ContainerInterface
 	 */
 	private function emptyContainer(): ContainerInterface {
-		$container = $this->createMock(ContainerInterface::class);
-		$container->method('has')->willReturn(false);
-		return $container;
+		return CmdbTestSupport::emptyContainer(test: $this);
 	}//end emptyContainer()
 
 	/**

tests/Unit/Repair/SeedCmdbImportPacksTest.php

@@ -163,8 +163,7 @@ public function testUneditedPacksAreUpdatedAndEditedOnesKept(): void {
 	 * @return void
 	 */
 	public function testWithoutTheStoreNothingIsSeeded(): void {
-		$container = $this->createMock(ContainerInterface::class);
-		$container->method('has')->willReturn(false);
+		$container = CmdbTestSupport::emptyContainer(test: $this);
 		$step = new SeedCmdbImportPacks(
 			profile: new CmdbImportProfile(container: $container),
 			store: new CmdbImportPackStore(container: $container),

tests/Unit/Service/Cmdb/CmdbImportProfileTest.php

@@ -23,8 +23,6 @@
 require_once __DIR__ . '/../../Support/CmdbTestSupport.php';
 require_once __DIR__ . '/../../Support/FakeMigrationPackService.php';
 
-use OCA\OpenRegister\Service\MigrationPack\MappingEngine;
-use OCA\OpenRegister\Service\MigrationPack\PackDefinitionValidator;
 use OCA\Stackiq\Exception\CmdbImportException;
 use OCA\Stackiq\Service\Cmdb\CmdbImportPackMarker;
 use OCA\Stackiq\Service\Cmdb\CmdbImportPackStore;
@@ -44,9 +42,7 @@ class CmdbImportProfileTest extends TestCase {
 	 * @return ContainerInterface
 	 */
 	private function emptyContainer(): ContainerInterface {
-		$container = $this->createMock(ContainerInterface::class);
-		$container->method('has')->willReturn(false);
-		return $container;
+		return CmdbTestSupport::emptyContainer(test: $this);
 	}//end emptyContainer()
 
 	/**
@@ -86,7 +82,7 @@ public function testEveryPackIsAValidOpenRegisterPack(): void {
 		$profile = new CmdbImportProfile(container: $this->emptyContainer());
 		$profile->load();
 
-		$validator = new PackDefinitionValidator();
+		$validator = CmdbTestSupport::newPackClass(class: 'PackDefinitionValidator');
 		foreach (CmdbImportProfile::TARGETS as $target) {
 			$pack = $profile->pack(target: $target);
 			$this->assertSame([], $validator->validate($pack), $target);
@@ -176,7 +172,7 @@ public function testTheLookupsMapThroughTheEngine(): void {
 		CmdbTestSupport::loadMigrationPack();
 		$profile = new CmdbImportProfile(container: $this->emptyContainer());
 		$profile->load();
-		$engine = new MappingEngine();
+		$engine = CmdbTestSupport::newPackClass(class: 'MappingEngine');
 
 		$usage = $engine->mapRow(
 			$profile->pack(target: 'usage'),
@@ -704,8 +700,9 @@ public function testWithoutTheStoreBeforeASeedTheShippedFilesAreUsed(): void {
 		$config = [];
 		$container = $this->createMock(ContainerInterface::class);
 		$appConfig = FakeMigrationPackService::appConfig(test: $this, values: $config);
-		$container->method('has')->willReturnCallback(fn (string $id): bool => $id === \OCP\IAppConfig::class);
-		$container->method('get')->willReturn($appConfig);
+		$factory = CmdbTestSupport::translationFactory(test: $this);
+		$container->method('has')->willReturnCallback(fn (string $id): bool => $id === \OCP\IAppConfig::class || $id === \OCP\L10N\IFactory::class);
+		$container->method('get')->willReturnCallback(fn (string $id): object => $id === \OCP\L10N\IFactory::class ? $factory : $appConfig);
 
 		$profile = new CmdbImportProfile(container: $container);
 		$overview = $profile->mappingOverview();
@@ -728,8 +725,9 @@ public function testWithoutTheStoreAfterASeedTheImportFailsClosed(): void {
 		$config = [CmdbImportPackStore::CONFIG_KEY => (string)json_encode(['stackiq-topdesk-module' => 41, 'stackiq-topdesk-usage' => 43])];
 		$container = $this->createMock(ContainerInterface::class);
 		$appConfig = FakeMigrationPackService::appConfig(test: $this, values: $config);
-		$container->method('has')->willReturnCallback(fn (string $id): bool => $id === \OCP\IAppConfig::class);
-		$container->method('get')->willReturn($appConfig);
+		$factory = CmdbTestSupport::translationFactory(test: $this);
+		$container->method('has')->willReturnCallback(fn (string $id): bool => $id === \OCP\IAppConfig::class || $id === \OCP\L10N\IFactory::class);
+		$container->method('get')->willReturnCallback(fn (string $id): object => $id === \OCP\L10N\IFactory::class ? $factory : $appConfig);
 
 		try {
 			(new CmdbImportProfile(container: $container))->load();

tests/Unit/Service/Cmdb/CmdbWorkbookReaderTest.php

@@ -43,8 +43,7 @@ class CmdbWorkbookReaderTest extends TestCase {
 	 */
 	private function profile(?string $directory = null): CmdbImportProfile {
 		CmdbTestSupport::loadMigrationPack();
-		$container = $this->createMock(ContainerInterface::class);
-		$container->method('has')->willReturn(false);
+		$container = CmdbTestSupport::emptyContainer(test: $this);
 
 		$profile = new CmdbImportProfile(container: $container, directory: $directory);
 		$profile->load();

tests/Unit/Service/Cmdb/OpenRegisterInstanceTest.php

@@ -0,0 +1,89 @@
+<?php
+
+/**
+ * Tests for OpenRegisterInstance.
+ *
+ * @category  Test
+ * @package   OCA\Stackiq\Tests\Unit\Service\Cmdb
+ * @author    Conduction b.v. <[email protected]>
+ * @copyright 2026 Conduction B.V.
+ * @license   EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12
+ * @link      https://github.com/ConductionNL/stackiq
+ *
+ * @spec openspec/changes/cmdb-export-import/tasks.md#task-3
+ *
+ * SPDX-FileCopyrightText: 2026 Conduction B.V. <[email protected]>
+ * SPDX-License-Identifier: EUPL-1.2
+ */
+
+declare(strict_types=1);
+
+namespace OCA\Stackiq\Tests\Unit\Service\Cmdb;
+
+use OCA\Stackiq\Service\Cmdb\OpenRegisterInstance;
+use OCP\IL10N;
+use OCP\L10N\IFactory;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\TestCase;
+use Psr\Container\ContainerInterface;
+
+/**
+ * An OpenRegister class is built with the arguments its constructor declares.
+ */
+#[CoversClass(OpenRegisterInstance::class)]
+class OpenRegisterInstanceTest extends TestCase {
+	/**
+	 * A class without constructor parameters (older OpenRegister) is built as is.
+	 *
+	 * @return void
+	 */
+	public function testAClassWithoutParametersIsBuilt(): void {
+		$object = (new OpenRegisterInstance(container: $this->createMock(ContainerInterface::class)))->build(class: \stdClass::class);
+
+		$this->assertInstanceOf(\stdClass::class, $object);
+	}//end testAClassWithoutParametersIsBuilt()
+
+	/**
+	 * A class that requires an IL10N (newer OpenRegister) gets OpenRegister's translations.
+	 *
+	 * @return void
+	 */
+	public function testAClassRequiringTranslationsGetsThem(): void {
+		$l10n = $this->createMock(IL10N::class);
+		$factory = $this->createMock(IFactory::class);
+		$factory->expects($this->once())->method('get')->with('openregister')->willReturn($l10n);
+		$container = $this->createMock(ContainerInterface::class);
+		$container->method('get')->with(IFactory::class)->willReturn($factory);
+		$class = new class($l10n) {
+			public function __construct(public readonly IL10N $l10n) {
+			}
+		};
+
+		$object = (new OpenRegisterInstance(container: $container))->build(class: $class::class);
+
+		$this->assertInstanceOf($class::class, $object);
+		$this->assertSame($l10n, $object->l10n);
+	}//end testAClassRequiringTranslationsGetsThem()
+
+	/**
+	 * A constructor needing anything else, or a container without the translation factory, gives null.
+	 *
+	 * @return void
+	 */
+	public function testAnUnbuildableClassGivesNull(): void {
+		$container = $this->createMock(ContainerInterface::class);
+		$other = new class('x') {
+			public function __construct(public readonly string $value) {
+			}
+		};
+		$this->assertNull((new OpenRegisterInstance(container: $container))->build(class: $other::class));
+
+		$noFactory = $this->createMock(ContainerInterface::class);
+		$noFactory->method('get')->willThrowException(new \RuntimeException('not in this container'));
+		$needsL10n = new class($this->createMock(IL10N::class)) {
+			public function __construct(public readonly IL10N $l10n) {
+			}
+		};
+		$this->assertNull((new OpenRegisterInstance(container: $noFactory))->build(class: $needsL10n::class));
+	}//end testAnUnbuildableClassGivesNull()
+}//end class

tests/Unit/Service/CmdbExportImportServiceTest.php

@@ -1075,9 +1075,13 @@ private function service(
 
 		[$archiveHandler, $objectMapper] = $this->archiveDoubles();
 		$container = $this->createMock(ContainerInterface::class);
-		$container->method('has')->willReturnCallback(fn (string $id): bool => $packs !== false && $id === CmdbImportPackStore::SERVICE_CLASS);
+		$container->method('has')->willReturnCallback(fn (string $id): bool => $id === \OCP\L10N\IFactory::class || ($packs !== false && $id === CmdbImportPackStore::SERVICE_CLASS));
 		$container->method('get')->willReturnCallback(
 			function (string $id) use ($objectService, $schemaMapper, $packs, $archiveHandler, $objectMapper) {
+				if ($id === \OCP\L10N\IFactory::class) {
+					return CmdbTestSupport::translationFactory(test: $this);
+				}
+
 				if ($id === ObjectServiceInterface::class) {
 					return $objectService;
 				}