From 96c21899ccc123f5c20afac0c138aa2a042fd0e9 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Tue, 3 Nov 2026 20:33:06 +0100 Subject: [PATCH 1/3] fix(organisations): never take over an existing organisation on a name match When an organisatie had no OpenRegister organisation yet, the sync looked one up by the slug its name gives and, on a match, wrote the organisatie's uuid onto that organisation: a registration named like an existing organisation took it over. ContactPersonHandler did the same for a contact person's organisation and then added the contact's user to it. The slug was also computed without OpenRegister's 100-character cut, so a long name missed the match and OpenRegister's own create overwrote the existing organisation. Now a taken slug is refused on both paths: nothing is created or changed, a neutral warning names the organisatie and the organisation holding the slug, and the sync counts it under slugConflicts. OpenRegister's OrganisationSlugConflictException (openregister#5462, a 409) is handled the same way. OrganisationSlug computes the slug exactly as OpenRegister does. `occ stackiq:organisation:link ` lets an administrator link the two explicitly. It refuses an unknown organisatie or organisation, an organisatie that already has one, and an organisation that belongs to another organisatie. Assisted-by: ClaudeCode:claude-opus-5-5 --- appinfo/info.xml | 4 + lib/Command/OrganisationLinkCommand.php | 111 ++++ lib/Service/OrganisatieService.php | 13 + lib/Service/OrganisationSlug.php | 56 ++ lib/Service/OrganizationSyncService.php | 193 ++++-- lib/Service/Stackiq/ContactPersonHandler.php | 50 +- tests/Stubs/Db/OrganisationMapper.php | 9 + .../OrganisationSlugConflictException.php | 35 ++ tests/Stubs/Service/OrganisationService.php | 17 + .../Command/OrganisationLinkCommandTest.php | 81 +++ .../OrganisatieServiceSlugConflictTest.php | 89 +++ ...rganizationSyncContactPersonGoldenTest.php | 1 + .../OrganizationSyncEntityGoldenTest.php | 84 +++ ...anizationSyncRelatedContactsGoldenTest.php | 1 + .../ContactPersonHandlerSlugConflictTest.php | 316 ++++++++++ .../golden/organizationSyncEntity.json | 554 +++++++++++++++--- 16 files changed, 1483 insertions(+), 131 deletions(-) create mode 100644 lib/Command/OrganisationLinkCommand.php create mode 100644 lib/Service/OrganisationSlug.php create mode 100644 tests/Stubs/OpenRegister/Exception/OrganisationSlugConflictException.php create mode 100644 tests/Unit/Command/OrganisationLinkCommandTest.php create mode 100644 tests/Unit/Service/OrganisatieServiceSlugConflictTest.php create mode 100644 tests/Unit/Service/Stackiq/ContactPersonHandlerSlugConflictTest.php diff --git a/appinfo/info.xml b/appinfo/info.xml index a1b6820d6..bb0cc6f63 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -344,6 +344,10 @@ Vrij en open source onder de EUPL-licentie. shared `contractNumber`: a wrong cross-app link is worse than an empty one. --> OCA\Stackiq\Command\ReferencesAuditCommand + + OCA\Stackiq\Command\OrganisationLinkCommand diff --git a/lib/Command/OrganisationLinkCommand.php b/lib/Command/OrganisationLinkCommand.php new file mode 100644 index 000000000..bf97111da --- /dev/null +++ b/lib/Command/OrganisationLinkCommand.php @@ -0,0 +1,111 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://conduction.nl + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Stackiq\Command; + +use OCA\Stackiq\Service\OrganizationSyncService; +use Psr\Container\ContainerInterface; +use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Input\InputArgument; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Output\OutputInterface; +use Throwable; + +/** + * `occ stackiq:organisation:link `. + * + * The organisation sync never takes over an OpenRegister organisation whose + * name gives the same slug: it leaves the organisatie unlinked and logs it. + * This command is how an administrator links the two when they are the same. + * + * @spec openspec/specs/organisatie-service/spec.md + * + * phpcs:disable CustomSniffs.Functions.NamedParameters + */ +class OrganisationLinkCommand extends Command { + + /** + * What each outcome means, for the console. + */ + private const MESSAGES = [ + 'linked' => 'Linked. The organisation now carries the organisatie uuid and owns the organisatie.', + 'no-organisatie' => 'No organisatie has this uuid.', + 'already-linked' => 'This organisatie already has an OpenRegister organisation.', + 'no-organisation' => 'No OpenRegister organisation has this uuid.', + 'organisation-in-use' => 'This OpenRegister organisation belongs to another organisatie.', + ]; + + /** + * Build the command. + * + * @param ContainerInterface $container Container, for the lazy sync service resolve. + */ + public function __construct( + private readonly ContainerInterface $container, + ) { + parent::__construct(); + }//end __construct() + + /** + * Declare the command. + * + * @return void + * + * @spec openspec/specs/organisatie-service/spec.md + */ + protected function configure(): void { + $this->setName('stackiq:organisation:link') + ->setDescription('Link an organisatie to an existing OpenRegister organisation.') + ->addArgument('organisatie', InputArgument::REQUIRED, 'The organisatie object uuid.') + ->addArgument('organisation', InputArgument::REQUIRED, 'The OpenRegister organisation uuid.'); + }//end configure() + + /** + * Make the link, or say why not. + * + * @param InputInterface $input Console input. + * @param OutputInterface $output Console output. + * + * @return int 0 when linked, 1 otherwise. + * + * @spec openspec/specs/organisatie-service/spec.md + */ + protected function execute(InputInterface $input, OutputInterface $output): int { + try { + $outcome = $this->container->get(OrganizationSyncService::class)->linkOrganisationEntity( + organisatieUuid: (string) $input->getArgument('organisatie'), + entityUuid: (string) $input->getArgument('organisation') + ); + } catch (Throwable $e) { + $output->writeln(messages: 'Could not link: ' . $e->getMessage() . ''); + return 1; + } + + $output->writeln(messages: self::MESSAGES[$outcome] ?? $outcome); + if ($outcome === 'linked') { + return 0; + } + + return 1; + }//end execute() +}//end class diff --git a/lib/Service/OrganisatieService.php b/lib/Service/OrganisatieService.php index 7c77a9a23..b76a4a36a 100644 --- a/lib/Service/OrganisatieService.php +++ b/lib/Service/OrganisatieService.php @@ -42,6 +42,12 @@ * @SuppressWarnings(PHPMD.UnusedPrivateMethod) */ class OrganisatieService { + /** + * OpenRegister's exception for a taken slug, by name (OrganisationSlug::CONFLICT_EXCEPTION; + * not imported, to keep this class's coupling down). + */ + private const SLUG_CONFLICT = 'OCA\\OpenRegister\\Exception\\OrganisationSlugConflictException'; + /** * OrganisatieService constructor. * @@ -71,6 +77,8 @@ public function __construct( * * @return object|null The created organisation entity or null on failure * + * OpenRegister's OrganisationSlugConflictException (another organisation has the name's slug) is passed on. + * * @spec openspec/specs/organisatie-service/spec.md */ public function createOrganisationInOpenRegister(array $objectData): ?object { @@ -118,6 +126,11 @@ public function createOrganisationInOpenRegister(array $objectData): ?object { return $organisationEntity; } catch (\Exception $e) { + // The caller decides what a taken slug means; every other failure ends here. + if (is_a($e, self::SLUG_CONFLICT) === true) { + throw $e; + } + $this->logger->error( 'OrganisatieService: Error creating organization entity', [ diff --git a/lib/Service/OrganisationSlug.php b/lib/Service/OrganisationSlug.php new file mode 100644 index 000000000..52cd6ee2e --- /dev/null +++ b/lib/Service/OrganisationSlug.php @@ -0,0 +1,56 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://conduction.nl + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Stackiq\Service; + +/** + * Mirrors OpenRegister's OrganisationService::generateSlug(): lower case, runs of + * other characters as one hyphen, no hyphen at either end, at most 100 characters. + * It also names OpenRegister's exception for a taken slug. + * + * @spec openspec/specs/organisatie-service/spec.md + */ +class OrganisationSlug { + + /** + * OpenRegister's exception for a new organisation whose slug is taken (openregister#5462). + * Named as a string: older OpenRegister releases do not have the class. + */ + public const CONFLICT_EXCEPTION = 'OCA\\OpenRegister\\Exception\\OrganisationSlugConflictException'; + + /** + * OpenRegister keeps the first 100 characters of a slug. + */ + private const MAX_LENGTH = 100; + + /** + * The slug of an organisation name. + * + * @param string $name The organisation name. + * + * @return string The slug. + * + * @spec openspec/specs/organisatie-service/spec.md + */ + public function fromName(string $name): string { + $slug = trim((string) preg_replace('/[^a-z0-9]+/', '-', strtolower($name)), '-'); + return substr($slug, 0, self::MAX_LENGTH); + }//end fromName() +}//end class diff --git a/lib/Service/OrganizationSyncService.php b/lib/Service/OrganizationSyncService.php index 7cc7e4d0a..7a3a6332c 100644 --- a/lib/Service/OrganizationSyncService.php +++ b/lib/Service/OrganizationSyncService.php @@ -992,7 +992,11 @@ private function ensureOrganisationEntity(object $organisationObject, array $run $organisationEntity = $this->organisationMapper->findByUuid($organisationId); return $this->refreshOrganisationEntity(organisationEntity: $organisationEntity, run: $run); } catch (\OCP\AppFramework\Db\DoesNotExistException $e) { - return $this->adoptOrganisationEntityBySlug(run: $run) ?? $this->createOrganisationEntity(run: $run); + if ($this->slugIsTaken(run: $run) === true) { + return null; + } + + return $this->createOrganisationEntity(run: $run); } } catch (\Exception $e) { $this->logger->error( @@ -1100,48 +1104,159 @@ private function refreshOrganisationEntity(object $organisationEntity, array $ru }//end refreshOrganisationEntity() /** - * Adopt an entity that has the organisation's slug but another uuid: it takes the object's uuid. + * Link an organisatie to an existing OpenRegister organisation, as an administrator decides. * - * @param array $run The ensure run (see ensureOrganisationEntity). + * The organisation takes the organisatie's uuid, which is what links the two, + * and becomes the organisatie object's owner. Refused when the organisatie is + * unknown or already has an organisation, when the organisation is unknown, or + * when the organisation is already another organisatie's. * - * @return object|null The adopted entity, or null when the object has no name or no entity has its slug + * @param string $organisatieUuid The organisatie object's uuid. + * @param string $entityUuid The OpenRegister organisation's uuid. + * + * @return string linked, no-organisatie, already-linked, no-organisation or organisation-in-use. + * + * @spec openspec/specs/organisatie-service/spec.md */ - private function adoptOrganisationEntityBySlug(array $run): ?object { - // Entity not found by UUID — try finding by slug before creating. - // This handles the case where the entity exists but with a different UUID (e.g. from slug collision). - $orgName = ($run['objectData']['name'] ?? ''); - if (empty($orgName) === true) { - return null; + public function linkOrganisationEntity(string $organisatieUuid, string $entityUuid): string { + $config = $this->settingsService->getVoorzieningenConfig(); + $run = [ + 'register' => (string) ($config['register'] ?? ''), + 'organizationSchema' => (string) ($config['organisatie_schema'] ?? ''), + ]; + $run['organisationObject'] = $this->findOrganisatie(run: $run, uuid: $organisatieUuid); + if ($run['organisationObject'] === null) { + return 'no-organisatie'; + } + + $outcome = $this->linkRefusal(run: $run, uuids: [$organisatieUuid, $entityUuid]); + if ($outcome !== null) { + return $outcome; + } + + $entity = $this->organisationMapper->findByUuid($entityUuid); + $entity->setUuid($organisatieUuid); + $this->organisationMapper->save($entity); + $this->makeEntityOwner(organisationEntity: $entity, run: $run); + $this->logger->info( + 'OrganizationSyncService: an administrator linked an organisatie to an OpenRegister organisation', + ['app' => 'stackiq', 'organisatieId' => $organisatieUuid, 'previousOrganisationUuid' => $entityUuid] + ); + + return 'linked'; + }//end linkOrganisationEntity() + + /** + * Why a link may not be made, or null when it may. + * + * @param array $run The register and organisatie schema. + * @param array{0: string, 1: string} $uuids The organisatie uuid and the organisation uuid. + * + * @return string|null already-linked, no-organisation, organisation-in-use, or null. + */ + private function linkRefusal(array $run, array $uuids): ?string { + [$organisatieUuid, $entityUuid] = $uuids; + if ($this->organisationExists(uuid: $organisatieUuid) === true) { + return 'already-linked'; + } + + if ($this->organisationExists(uuid: $entityUuid) === false) { + return 'no-organisation'; } - $slug = strtolower(preg_replace('/[^a-z0-9]+/', '-', strtolower($orgName))); - $slug = trim($slug, '-'); + if ($this->findOrganisatie(run: $run, uuid: $entityUuid) !== null) { + return 'organisation-in-use'; + } + + return null; + }//end linkRefusal() + + /** + * Whether an OpenRegister organisation has this uuid. + * + * @param string $uuid The uuid. + * + * @return bool + */ + private function organisationExists(string $uuid): bool { try { - $organisationEntity = $this->organisationMapper->findBySlug($slug); - $this->logger->info( - 'OrganizationSyncService: Found existing entity by slug, updating UUID to match object', - [ - 'app' => 'stackiq', - 'organisatieId' => $run['organisationId'], - 'oldEntityUuid' => $organisationEntity->getUuid(), - 'slug' => $slug, - ] + $this->organisationMapper->findByUuid($uuid); + return true; + } catch (\OCP\AppFramework\Db\DoesNotExistException $e) { + return false; + } + }//end organisationExists() + + /** + * The organisatie object with this uuid, or null. + * + * @param array $run The register and organisatie schema. + * @param string $uuid The uuid. + * + * @return object|null + */ + private function findOrganisatie(array $run, string $uuid): ?object { + try { + return $this->objectService->find( + id: $uuid, + register: $run['register'], + schema: $run['organizationSchema'], + _rbac: false, + _multitenancy: false ); + } catch (\OCP\AppFramework\Db\DoesNotExistException $e) { + return null; + } + }//end findOrganisatie() - // Update the entity's UUID to match the object UUID so future lookups work. - $organisationEntity->setUuid($run['organisationId']); - $this->organisationMapper->save($organisationEntity); - $run['stats']['entitiesUpdated']++; + /** + * Whether another OpenRegister organisation already has the slug this organisatie's name gives. + * + * Such an organisation is never taken over on a name match: the organisatie is + * left without an entity, logged and counted, for an administrator to link + * explicitly (occ stackiq:organisation:link). A missing or "Unknown" name gets a + * uuid-based name when the entity is created, so it is not checked. + * + * @param array $run The ensure run (see ensureOrganisationEntity). + * + * @return bool True when the slug is taken and nothing may be created. + */ + private function slugIsTaken(array $run): bool { + $orgName = (string) ($run['objectData']['name'] ?? ''); + if ($orgName === '' || $orgName === 'Unknown') { + return false; + } - // Update organisatie object owner to this entity. - $this->makeEntityOwner(organisationEntity: $organisationEntity, run: $run); + $slug = (new OrganisationSlug())->fromName(name: $orgName); + try { + $existing = $this->organisationMapper->findBySlug($slug); + } catch (\OCP\AppFramework\Db\DoesNotExistException $e) { + return false; + } - return $organisationEntity; - } catch (\OCP\AppFramework\Db\DoesNotExistException $slugEx) { - // Not found by slug either — proceed with creation. - return null; - }//end try - }//end adoptOrganisationEntityBySlug() + $this->refuseSlugConflict(run: $run, existingUuid: $existing->getUuid()); + return true; + }//end slugIsTaken() + + /** + * Log and count an organisatie left unlinked because its slug is taken. + * + * @param array $run The ensure run (see ensureOrganisationEntity). + * @param string|null $existingUuid The uuid of the organisation holding the slug, when known. + * + * @return void + */ + private function refuseSlugConflict(array $run, ?string $existingUuid): void { + $this->logger->warning( + 'OrganizationSyncService: an organisation with this name already exists; the organisatie is left unlinked for an administrator', + [ + 'app' => 'stackiq', + 'organisatieId' => $run['organisationId'], + 'existingOrganisationUuid' => $existingUuid, + ] + ); + $run['stats']['slugConflicts'][] = $run['organisationId']; + }//end refuseSlugConflict() /** * Create the organisation entity, mail the new organisation and make the entity the object's owner. @@ -1161,7 +1276,17 @@ private function createOrganisationEntity(array $run): ?object { ] ); - $organisationEntity = $this->organisationService->createOrganisationInOpenRegister($run['objectData']); + try { + $organisationEntity = $this->organisationService->createOrganisationInOpenRegister($run['objectData']); + } catch (\RuntimeException $e) { + if (is_a($e, OrganisationSlug::CONFLICT_EXCEPTION) === false) { + throw $e; + } + + $this->refuseSlugConflict(run: $run, existingUuid: null); + return null; + } + if (empty($organisationEntity) === true) { $this->logger->error( '❌ ORGANISATION ENTITY CREATION FAILED', diff --git a/lib/Service/Stackiq/ContactPersonHandler.php b/lib/Service/Stackiq/ContactPersonHandler.php index 13d312fad..ca4717a72 100644 --- a/lib/Service/Stackiq/ContactPersonHandler.php +++ b/lib/Service/Stackiq/ContactPersonHandler.php @@ -18,6 +18,7 @@ namespace OCA\Stackiq\Service\Stackiq; +use OCA\Stackiq\Service\OrganisationSlug; use OCA\Stackiq\Service\SymfonyEmailService; use OCP\App\IAppManager; use OCP\IAppConfig; @@ -2396,9 +2397,10 @@ public function addUserToOrganizationEntity( $organisation = $this->ensureOrganizationEntity(organizationUuid: $organizationUuid); + // The reason there is none is logged by ensureOrganizationEntity(). if ($organisation === null) { - $this->_logger->error( - 'ContactPersonHandler: Failed to create organization entity', + $this->_logger->info( + 'ContactPersonHandler: No organization entity, user not added', [ 'organizationUuid' => $organizationUuid, ] @@ -2522,23 +2524,14 @@ private function ensureOrganizationEntity(string $organizationUuid): ?\OCA\OpenR $description = $organizationData['description'] ?? $organizationData['longDescription'] ?? null; $organizationDescription = $description ?? $organizationData['description'] ?? ''; - // Check if an entity with the same slug already exists (prevents unique constraint violation). + // Another organisation with this name's slug is never taken over: the + // organisatie stays without an entity until an administrator links it. try { - $slug = strtolower(preg_replace('/[^a-z0-9]+/', '-', strtolower($organizationName))); - $slug = trim($slug, '-'); - $existingBySlug = $organisationMapper->findBySlug($slug); - $this->_logger->info( - 'ContactPersonHandler: Organization entity already exists (found by slug)', - [ - 'organizationUuid' => $organizationUuid, - 'slug' => $slug, - 'existingUuid' => $existingBySlug->getUuid(), - 'existingName' => $existingBySlug->getName(), - ] - ); - return $existingBySlug; + $existingBySlug = $organisationMapper->findBySlug((new OrganisationSlug())->fromName(name: $organizationName)); + $this->logSlugConflict(organizationUuid: $organizationUuid, existingUuid: $existingBySlug->getUuid()); + return null; } catch (\OCP\AppFramework\Db\DoesNotExistException $e) { - // No existing entity by slug — proceed with creation. + // No organisation has this slug: create one. } $this->_logger->info( @@ -2572,6 +2565,11 @@ private function ensureOrganizationEntity(string $organizationUuid): ?\OCA\OpenR return $organisation; } catch (\Exception $e) { + if (is_a($e, OrganisationSlug::CONFLICT_EXCEPTION) === true) { + $this->logSlugConflict(organizationUuid: $organizationUuid, existingUuid: null); + return null; + } + $this->_logger->error( 'ContactPersonHandler: Failed to ensure organization entity', [ @@ -2586,6 +2584,24 @@ private function ensureOrganizationEntity(string $organizationUuid): ?\OCA\OpenR }//end try }//end ensureOrganizationEntity() + /** + * Log an organisatie left without an entity because its name's slug is taken. + * + * @param string $organizationUuid The organisatie uuid. + * @param string|null $existingUuid The uuid of the organisation holding the slug, when known. + * + * @return void + */ + private function logSlugConflict(string $organizationUuid, ?string $existingUuid): void { + $this->_logger->warning( + 'ContactPersonHandler: an organisation with this name already exists; the organisatie is left unlinked for an administrator', + [ + 'organizationUuid' => $organizationUuid, + 'existingOrganisationUuid' => $existingUuid, + ] + ); + }//end logSlugConflict() + /** * Strips spaces and non-alphanumeric characters from voornaam/achternaam * and lowercases the result so they can be safely concatenated into a diff --git a/tests/Stubs/Db/OrganisationMapper.php b/tests/Stubs/Db/OrganisationMapper.php index e76182296..6adac68d8 100644 --- a/tests/Stubs/Db/OrganisationMapper.php +++ b/tests/Stubs/Db/OrganisationMapper.php @@ -49,4 +49,13 @@ abstract public function findBySlug(string $slug): Organisation; */ abstract public function save(Organisation $entity): Organisation; + /** + * Update an organisation entity. + * + * @param Organisation $entity The entity to update. + * + * @return Organisation + */ + abstract public function update(Organisation $entity): Organisation; + }//end class diff --git a/tests/Stubs/OpenRegister/Exception/OrganisationSlugConflictException.php b/tests/Stubs/OpenRegister/Exception/OrganisationSlugConflictException.php new file mode 100644 index 000000000..b969b65d9 --- /dev/null +++ b/tests/Stubs/OpenRegister/Exception/OrganisationSlugConflictException.php @@ -0,0 +1,35 @@ + + * SPDX-License-Identifier: EUPL-1.2 + * + * @category Test + * @package OCA\Stackiq\Tests\Stubs + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Exception; + +use RuntimeException; + +/** + * A new organisation's slug is already taken. + */ +class OrganisationSlugConflictException extends RuntimeException { + + /** + * @param string $slug The taken slug. + * @param \Throwable|null $previous The cause. + */ + public function __construct(public readonly string $slug, ?\Throwable $previous = null) { + parent::__construct(message: 'An organisation with this name already exists.', code: 409, previous: $previous); + }//end __construct() +}//end class diff --git a/tests/Stubs/Service/OrganisationService.php b/tests/Stubs/Service/OrganisationService.php index d535023cf..bb1695b52 100644 --- a/tests/Stubs/Service/OrganisationService.php +++ b/tests/Stubs/Service/OrganisationService.php @@ -79,4 +79,21 @@ abstract public function joinOrganisation(string $organisationUuid, ?string $tar */ abstract public function leaveOrganisation(string $organisationUuid, ?string $targetUserId = null): bool; + /** + * Create an organisation; with addCurrentUser the signed-in user becomes its owner and a member. + * + * @param string $name The name. + * @param string $description The description. + * @param bool $addCurrentUser Whether the signed-in user owns and joins it. + * @param string $uuid The uuid to use. + * + * @return Organisation + */ + abstract public function createOrganisation( + string $name, + string $description = '', + bool $addCurrentUser = true, + string $uuid = '', + ): Organisation; + }//end class diff --git a/tests/Unit/Command/OrganisationLinkCommandTest.php b/tests/Unit/Command/OrganisationLinkCommandTest.php new file mode 100644 index 000000000..42fe7601e --- /dev/null +++ b/tests/Unit/Command/OrganisationLinkCommandTest.php @@ -0,0 +1,81 @@ + + * @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/specs/organisatie-service/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Stackiq\Tests\Unit\Command; + +use OCA\Stackiq\Command\OrganisationLinkCommand; +use OCA\Stackiq\Service\OrganizationSyncService; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use RuntimeException; +use Symfony\Component\Console\Tester\CommandTester; + +/** + * The command passes both uuids on and reports the outcome. + * + * @covers \OCA\Stackiq\Command\OrganisationLinkCommand + * + * @spec openspec/specs/organisatie-service/spec.md + */ +class OrganisationLinkCommandTest extends TestCase { + + /** + * Each outcome, its exit code and what the console says. + * + * @return array + */ + public static function outcomes(): array { + return [ + 'linked' => ['linked', 0, 'Linked.'], + 'no organisatie' => ['no-organisatie', 1, 'No organisatie has this uuid.'], + 'already linked' => ['already-linked', 1, 'already has an OpenRegister organisation'], + 'no organisation' => ['no-organisation', 1, 'No OpenRegister organisation has this uuid.'], + 'organisation in use' => ['organisation-in-use', 1, 'belongs to another organisatie'], + 'failure' => [new RuntimeException('db down'), 1, 'Could not link: db down'], + ]; + }//end outcomes() + + /** + * The outcome is reported with its exit code, for the uuids given. + * + * @param string|RuntimeException $outcome What the sync service answers. + * @param int $code The exit code. + * @param string $text Text the console shows. + * + * @return void + * + * @dataProvider outcomes + */ + public function testTheOutcomeIsReported(string|RuntimeException $outcome, int $code, string $text): void { + $sync = $this->createMock(OrganizationSyncService::class); + $call = $sync->expects(self::once())->method('linkOrganisationEntity')->with('org-uuid', 'or-uuid'); + if ($outcome instanceof RuntimeException) { + $call->willThrowException($outcome); + } else { + $call->willReturn($outcome); + } + + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->with(OrganizationSyncService::class)->willReturn($sync); + + $tester = new CommandTester(new OrganisationLinkCommand($container)); + $tester->execute(['organisatie' => 'org-uuid', 'organisation' => 'or-uuid']); + + self::assertSame($code, $tester->getStatusCode()); + self::assertStringContainsString($text, $tester->getDisplay()); + }//end testTheOutcomeIsReported() +}//end class diff --git a/tests/Unit/Service/OrganisatieServiceSlugConflictTest.php b/tests/Unit/Service/OrganisatieServiceSlugConflictTest.php new file mode 100644 index 000000000..f0b9eeb55 --- /dev/null +++ b/tests/Unit/Service/OrganisatieServiceSlugConflictTest.php @@ -0,0 +1,89 @@ + + * @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/specs/organisatie-service/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Stackiq\Tests\Unit\Service; + +use OCA\OpenRegister\Service\OrganisationService; +use OCA\Stackiq\Service\OrganisatieService; +use OCP\App\IAppManager; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use Psr\Log\NullLogger; +use ReflectionClass; + +if (class_exists('OCA\\OpenRegister\\Exception\\OrganisationSlugConflictException') === false) { + require_once __DIR__ . '/../../Stubs/OpenRegister/Exception/OrganisationSlugConflictException.php'; +} + +/** + * A taken slug reaches the caller; any other failure still ends as null. + * + * @covers \OCA\Stackiq\Service\OrganisatieService + * + * @spec openspec/specs/organisatie-service/spec.md + */ +class OrganisatieServiceSlugConflictTest extends TestCase { + + /** + * The refusal is passed on, not swallowed. + * + * @return void + */ + public function testATakenSlugIsPassedOn(): void { + $this->expectException(\OCA\OpenRegister\Exception\OrganisationSlugConflictException::class); + + $this->service(failure: new \OCA\OpenRegister\Exception\OrganisationSlugConflictException(slug: 'gemeente-a')) + ->createOrganisationInOpenRegister(['id' => 'org-1', 'name' => 'Gemeente A']); + }//end testATakenSlugIsPassedOn() + + /** + * Another failure still ends as null. + * + * @return void + */ + public function testAnotherFailureEndsAsNull(): void { + self::assertNull( + $this->service(failure: new \RuntimeException('db down'))->createOrganisationInOpenRegister(['id' => 'org-1', 'name' => 'Gemeente A']) + ); + }//end testAnotherFailureEndsAsNull() + + /** + * The service over an OpenRegister whose createOrganisation() throws. + * + * @param \Throwable $failure What createOrganisation() throws. + * + * @return OrganisatieService + */ + private function service(\Throwable $failure): OrganisatieService { + $organisations = $this->createMock(OrganisationService::class); + $organisations->method('getActiveOrganisation')->willReturn(null); + $organisations->method('createOrganisation')->willThrowException($failure); + + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->willReturn($organisations); + $apps = $this->createMock(IAppManager::class); + $apps->method('isEnabledForUser')->willReturn(true); + + $reflection = new ReflectionClass(OrganisatieService::class); + $service = $reflection->newInstanceWithoutConstructor(); + foreach (['container' => $container, 'logger' => new NullLogger(), 'appManager' => $apps] as $name => $value) { + $reflection->getProperty($name)->setValue($service, $value); + } + + return $service; + }//end service() +}//end class diff --git a/tests/Unit/Service/OrganizationSyncContactPersonGoldenTest.php b/tests/Unit/Service/OrganizationSyncContactPersonGoldenTest.php index 53f62cdd3..a3e40f167 100644 --- a/tests/Unit/Service/OrganizationSyncContactPersonGoldenTest.php +++ b/tests/Unit/Service/OrganizationSyncContactPersonGoldenTest.php @@ -40,6 +40,7 @@ * Golden cases for the contact person object sync. * * @covers \OCA\Stackiq\Service\OrganizationSyncService + * @uses \OCA\Stackiq\Service\OrganisationSlug * * @spec openspec/specs/organization-sync/spec.md */ diff --git a/tests/Unit/Service/OrganizationSyncEntityGoldenTest.php b/tests/Unit/Service/OrganizationSyncEntityGoldenTest.php index 4d9a86a69..c1f9336ff 100644 --- a/tests/Unit/Service/OrganizationSyncEntityGoldenTest.php +++ b/tests/Unit/Service/OrganizationSyncEntityGoldenTest.php @@ -33,12 +33,17 @@ use OCP\AppFramework\Db\DoesNotExistException; use OCP\IUser; use PHPUnit\Framework\TestCase; + +if (class_exists('OCA\\OpenRegister\\Exception\\OrganisationSlugConflictException') === false) { + require_once __DIR__ . '/../../Stubs/OpenRegister/Exception/OrganisationSlugConflictException.php'; +} use Psr\Log\AbstractLogger; /** * Golden cases for the organisation entity sync. * * @covers \OCA\Stackiq\Service\OrganizationSyncService + * @uses \OCA\Stackiq\Service\OrganisationSlug * * @spec openspec/specs/organization-sync/spec.md */ @@ -294,6 +299,10 @@ function (...$args) { $mapper->method('findBySlug')->willReturnCallback( function (string $slug) { $this->trace[] = ['mapper.findBySlug', $slug]; + if (array_key_exists('slugIndex', $this->plan) === true) { + return $this->plan['slugIndex'][$slug] ?? throw new \OCP\AppFramework\Db\DoesNotExistException('no slug'); + } + $answer = array_shift($this->plan['slug']); if ($answer instanceof \Throwable) { throw $answer; @@ -385,6 +394,10 @@ private function organisationService(): OrganisatieService { $service->method('createOrganisationInOpenRegister')->willReturnCallback( function (array $data) { $this->trace[] = ['organisations.create', $data]; + if (($this->plan['created'] ?? null) instanceof \Throwable) { + throw $this->plan['created']; + } + return array_key_exists('created', $this->plan) === true ? $this->plan['created'] : $this->orgEntity(uuid: 'new-entity', active: true); } ); @@ -571,6 +584,11 @@ public static function entityCases(): array { 'mapper fails' => [['name' => 'Gemeente K'], true, fn ($t) => ['org' => [new \RuntimeException('db down')]]], 'owner save fails' => [['name' => 'Gemeente L'], true, fn ($t) => ['org' => [$t->orgEntity('o-1', true)], 'save' => [new \RuntimeException('save failed')]]], 'settings fail' => [['name' => 'Gemeente M'], true, fn ($t) => ['config' => new \RuntimeException('settings down')]], + 'same name as another organisation' => [['name' => 'Gemeente A', 'status' => 'actief'], true, fn ($t) => ['org' => [$missing()], 'slugIndex' => ['gemeente-a' => $t->orgEntity('o-other', true)]]], + 'long name, slug cut at 100' => [['name' => 'Gemeente aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', 'status' => 'actief'], true, fn ($t) => ['org' => [$missing()], 'slugIndex' => ['gemeente-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' => $t->orgEntity('o-other', true)]]], + 'long name, slug free' => [['name' => 'Gemeente aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', 'status' => 'actief'], true, fn ($t) => ['org' => [$missing()], 'slugIndex' => []]], + 'OpenRegister refuses the slug' => [['name' => 'Gemeente P', 'status' => 'actief'], true, fn ($t) => ['org' => [$missing()], 'slugIndex' => [], 'created' => new \OCA\OpenRegister\Exception\OrganisationSlugConflictException(slug: 'gemeente-p')]], + 'named Unknown' => [['name' => 'Unknown', 'status' => 'actief'], true, fn ($t) => ['org' => [$missing()], 'slugIndex' => ['unknown' => $t->orgEntity('o-other', true)]]], ]; }//end entityCases() @@ -601,6 +619,72 @@ public function testEnsureEntity(array $object, bool $sendEmails, \Closure $plan }//end testEnsureEntity() + /** + * A registration named like an existing organisation neither takes over that + * organisation (its uuid, its owner link) nor creates a second one. + * + * @return void + */ + public function testASameNamedRegistrationDoesNotCaptureTheExistingOrganisation(): void { + foreach (['Gemeente A' => 'gemeente-a', 'Gemeente ' . str_repeat('a', 120) => substr('gemeente-' . str_repeat('a', 120), 0, 100)] as $name => $slug) { + $this->trace = []; + $this->plan = [ + 'find' => [null], + 'save' => [], + 'org' => [new \OCP\AppFramework\Db\DoesNotExistException('missing')], + 'slugIndex' => [$slug => $this->orgEntity(uuid: 'o-other', active: true)], + ]; + $stats = ['entitiesCreated' => 0, 'entitiesUpdated' => 0]; + $entity = $this->service()->ensureOrganisationEntityPublic($this->entity(object: ['name' => $name, 'status' => 'actief'], uuid: 'o-1'), $stats, true); + + $writes = array_values(array_filter($this->trace, fn (array $call) => in_array($call[0], ['entity.setUuid', 'mapper.save', 'organisations.create', 'objects.saveObject', 'entity.setOwner', 'entity.setOrganisation'], true))); + self::assertNull($entity, $name); + self::assertSame([], $writes, $name); + self::assertSame(['o-1'], $stats['slugConflicts'] ?? null, $name); + } + }//end testASameNamedRegistrationDoesNotCaptureTheExistingOrganisation() + + /** + * The link outcomes, and what a link writes. + * + * @return array}> + */ + public static function linkCases(): array { + $missing = fn () => new \OCP\AppFramework\Db\DoesNotExistException('missing'); + $object = fn ($t, string $uuid) => $t->entity(object: ['name' => 'Gemeente A'], uuid: $uuid); + return [ + 'linked' => ['linked', fn ($t) => ['find' => [$object($t, 'o-1'), null], 'org' => [$missing(), $t->orgEntity('o-old', true), $t->orgEntity('o-old', true)]], ['entity.setUuid', 'mapper.save', 'objects.saveObject']], + 'no organisatie' => ['no-organisatie', fn ($t) => ['find' => [null]], []], + 'already linked' => ['already-linked', fn ($t) => ['find' => [$object($t, 'o-1')], 'org' => [$t->orgEntity('o-1', true)]], []], + 'no organisation' => ['no-organisation', fn ($t) => ['find' => [$object($t, 'o-1')], 'org' => [$missing(), $missing()]], []], + 'organisation in use' => ['organisation-in-use', fn ($t) => ['find' => [$object($t, 'o-1'), $object($t, 'o-old')], 'org' => [$missing(), $t->orgEntity('o-old', true)]], []], + ]; + }//end linkCases() + + /** + * An administrator's link writes only when every check passes. + * + * @param string $outcome The expected outcome. + * @param \Closure $plan Builds the doubles' answers. + * @param array $writes The writes expected, in order. + * + * @return void + * + * @dataProvider linkCases + */ + public function testLinkOrganisationEntity(string $outcome, \Closure $plan, array $writes): void { + $this->trace = []; + $this->plan = array_merge(['find' => [], 'save' => [], 'org' => [], 'slug' => []], $plan($this)); + + self::assertSame($outcome, $this->service()->linkOrganisationEntity('o-1', 'o-old')); + + $made = array_values(array_filter($this->trace, fn (array $call) => in_array($call[0], ['entity.setUuid', 'mapper.save', 'objects.saveObject'], true))); + self::assertSame($writes, array_column($made, 0)); + if ($outcome === 'linked') { + self::assertContains(['entity.setUuid', 'o-old', 'o-1'], $this->trace); + } + }//end testLinkOrganisationEntity() + /** * Compare a case with the golden file, or record it. * diff --git a/tests/Unit/Service/OrganizationSyncRelatedContactsGoldenTest.php b/tests/Unit/Service/OrganizationSyncRelatedContactsGoldenTest.php index 9928614e1..ce265069e 100644 --- a/tests/Unit/Service/OrganizationSyncRelatedContactsGoldenTest.php +++ b/tests/Unit/Service/OrganizationSyncRelatedContactsGoldenTest.php @@ -39,6 +39,7 @@ * Golden cases for the related contact person sync. * * @covers \OCA\Stackiq\Service\OrganizationSyncService + * @uses \OCA\Stackiq\Service\OrganisationSlug * * @spec openspec/specs/organization-sync/spec.md */ diff --git a/tests/Unit/Service/Stackiq/ContactPersonHandlerSlugConflictTest.php b/tests/Unit/Service/Stackiq/ContactPersonHandlerSlugConflictTest.php new file mode 100644 index 000000000..64da8c454 --- /dev/null +++ b/tests/Unit/Service/Stackiq/ContactPersonHandlerSlugConflictTest.php @@ -0,0 +1,316 @@ + + * @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/specs/sc-handlers/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Stackiq\Tests\Unit\Service\Stackiq; + +use OCA\OpenRegister\Contract\ObjectServiceInterface; +use OCA\OpenRegister\Db\ObjectEntity; +use OCA\OpenRegister\Db\Organisation; +use OCA\OpenRegister\Db\OrganisationMapper; +use OCA\Stackiq\Service\SettingsService; +use OCA\Stackiq\Service\Stackiq\ContactPersonHandler; +use OCA\Stackiq\Service\SymfonyEmailService; +use OCP\App\IAppManager; +use OCP\AppFramework\Db\DoesNotExistException; +use OCP\IAppConfig; +use OCP\IConfig; +use OCP\IGroupManager; +use OCP\IUserManager; +use OCP\Security\ISecureRandom; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use Psr\Log\AbstractLogger; + +if (class_exists('OCA\\OpenRegister\\Exception\\OrganisationSlugConflictException') === false) { + require_once __DIR__ . '/../../../Stubs/OpenRegister/Exception/OrganisationSlugConflictException.php'; +} + +/** + * The contact person's user joins no organisation it does not belong to. + * + * @covers \OCA\Stackiq\Service\Stackiq\ContactPersonHandler + * @uses \OCA\Stackiq\Service\OrganisationSlug + * + * @spec openspec/specs/sc-handlers/spec.md + */ +class ContactPersonHandlerSlugConflictTest extends TestCase { + + /** + * The slugs findBySlug() was asked for. + * + * @var array + */ + private array $slugs = []; + + /** + * The organisations written. + * + * @var array + */ + private array $writes = []; + + /** + * The log levels and messages. + * + * @var array + */ + public array $logs = []; + + /** + * The organisations created. + * + * @var array + */ + private array $created = []; + + /** + * A same-named organisation is not joined, and no second one is created. + * + * @return void + */ + public function testASameNamedOrganisationIsNotJoined(): void { + $name = 'Gemeente ' . str_repeat('a', 120); + $this->handler(name: $name, slugTakenBy: 'o-other')->addUserToOrganizationEntity($this->contact(), 'new@gemeente.nl'); + + self::assertSame([substr('gemeente-' . str_repeat('a', 120), 0, 100)], $this->slugs); + self::assertSame([], $this->writes); + self::assertSame([], $this->created); + }//end testASameNamedOrganisationIsNotJoined() + + /** + * OpenRegister refusing the taken slug leaves the user out as well. + * + * @return void + */ + public function testARefusedSlugJoinsNothing(): void { + $this->handler(name: 'Gemeente B', slugTakenBy: null, refuse: true)->addUserToOrganizationEntity($this->contact(), 'new@gemeente.nl'); + + self::assertSame(['Gemeente B'], $this->created); + self::assertSame([], $this->writes); + self::assertContains('warning', array_column($this->logs, 0), 'a refused slug is a neutral warning, not an error'); + self::assertNotContains('error', array_column($this->logs, 0)); + }//end testARefusedSlugJoinsNothing() + + /** + * A free slug creates the organisation and adds the user to it. + * + * @return void + */ + public function testAFreeSlugCreatesTheOrganisationAndAddsTheUser(): void { + $this->handler(name: 'Gemeente C', slugTakenBy: null)->addUserToOrganizationEntity($this->contact(), 'new@gemeente.nl'); + + self::assertSame(['gemeente-c'], $this->slugs); + self::assertSame(['Gemeente C'], $this->created); + self::assertSame(['org-1'], $this->writes); + }//end testAFreeSlugCreatesTheOrganisationAndAddsTheUser() + + /** + * A contact person of organisatie org-1. + * + * @return object + */ + private function contact(): object { + return new class() { + /** + * @return string + */ + public function getId(): string { + return 'contact-1'; + } + + /** + * @return array + */ + public function getObject(): array { + return ['organisation' => 'org-1']; + } + }; + }//end contact() + + /** + * The handler over an organisatie org-1 that has no entity yet. + * + * @param string $name The organisatie's name. + * @param string|null $slugTakenBy The uuid of an organisation holding the slug, or null when free. + * @param bool $refuse Whether OpenRegister refuses the create as a taken slug. + * + * @return ContactPersonHandler + */ + private function handler(string $name, ?string $slugTakenBy, bool $refuse = false): ContactPersonHandler { + $mapper = $this->createMock(OrganisationMapper::class); + $mapper->method('findByUuid')->willThrowException(new DoesNotExistException('missing')); + $mapper->method('findBySlug')->willReturnCallback( + function (string $slug) use ($slugTakenBy) { + $this->slugs[] = $slug; + if ($slugTakenBy === null) { + throw new DoesNotExistException('no slug'); + } + + return $this->organisation(uuid: $slugTakenBy); + } + ); + foreach (['update', 'save'] as $write) { + $mapper->method($write)->willReturnCallback( + function ($organisation) { + $this->writes[] = $organisation->getUuid(); + return $organisation; + } + ); + } + + $organisatie = $this->createMock(ObjectEntity::class); + $organisatie->method('getObject')->willReturn(['name' => $name]); + $objects = $this->createMock(ObjectServiceInterface::class); + $objects->method('find')->willReturn($organisatie); + + $organisations = new class($this, $refuse) { + /** + * @param ContactPersonHandlerSlugConflictTest $test The test. + * @param bool $refuse Whether to refuse. + */ + public function __construct(private ContactPersonHandlerSlugConflictTest $test, private bool $refuse) { + } + + /** + * @param mixed ...$args The organisation, by name. + * + * @return Organisation + */ + public function createOrganisation(...$args): Organisation { + return $this->test->create(name: (string) ($args['name'] ?? $args[0]), refuse: $this->refuse); + } + }; + + $settings = $this->createMock(SettingsService::class); + $settings->method('getVoorzieningenConfig')->willReturn(['register' => '5', 'organisatie_schema' => '1']); + + $apps = $this->createMock(IAppManager::class); + $apps->method('getInstalledApps')->willReturn(['openregister']); + + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->willReturnCallback( + fn (string $id) => [ + 'OCA\\OpenRegister\\Db\\OrganisationMapper' => $mapper, + 'OCA\OpenRegister\Service\ObjectService' => $objects, + 'OCA\Stackiq\Service\SettingsService' => $settings, + 'OCA\\OpenRegister\\Service\\OrganisationService' => $organisations, + ][$id] ?? null + ); + + return new ContactPersonHandler( + $this->createMock(IUserManager::class), + $this->createMock(ISecureRandom::class), + $this->createMock(IGroupManager::class), + $this->createMock(IAppConfig::class), + $container, + $apps, + $this->logger(), + $this->createMock(SymfonyEmailService::class), + $this->createMock(IConfig::class) + ); + }//end handler() + + /** + * A logger that keeps each level and message. + * + * @return AbstractLogger + */ + private function logger(): AbstractLogger { + return new class($this) extends AbstractLogger { + /** + * @param ContactPersonHandlerSlugConflictTest $test The test. + */ + public function __construct(private ContactPersonHandlerSlugConflictTest $test) { + } + + /** + * @param mixed $level The level. + * @param string|\Stringable $message The message. + * @param array $context The context. + * + * @return void + */ + public function log($level, string|\Stringable $message, array $context = []): void { + $this->test->logs[] = [(string) $level, (string) $message]; + } + }; + }//end logger() + + /** + * Record a create and answer it, or refuse it as a taken slug. + * + * @param string $name The organisation name. + * @param bool $refuse Whether to refuse. + * + * @return Organisation The created organisation. + */ + public function create(string $name, bool $refuse): Organisation { + $this->created[] = $name; + if ($refuse === true) { + throw new \OCA\OpenRegister\Exception\OrganisationSlugConflictException(slug: 'taken'); + } + + return $this->organisation(uuid: 'org-1'); + }//end create() + + /** + * An organisation entity double with users. + * + * @param string $uuid The uuid. + * + * @return Organisation + */ + private function organisation(string $uuid): Organisation { + return new class($uuid) extends Organisation { + /** + * @var array + */ + private array $members = []; + + /** + * @param string $entityUuid The uuid. + */ + public function __construct(private string $entityUuid) { + } + + /** + * @return string + */ + public function getUuid(): string { + return $this->entityUuid; + } + + /** + * @param string $name The method. + * @param array $args The arguments. + * + * @return mixed + */ + public function __call(string $name, array $args) { + if ($name === 'getUsers') { + return $this->members; + } + + if ($name === 'setUsers') { + $this->members = $args[0]; + } + + return null; + } + }; + }//end organisation() +}//end class diff --git a/tests/Unit/Service/golden/organizationSyncEntity.json b/tests/Unit/Service/golden/organizationSyncEntity.json index 02b4560ab..c3900893b 100644 --- a/tests/Unit/Service/golden/organizationSyncEntity.json +++ b/tests/Unit/Service/golden/organizationSyncEntity.json @@ -899,13 +899,13 @@ ] }, "entity: adopted by slug": { - "returned": [ - "o-1", - true - ], + "returned": null, "stats": { "entitiesCreated": 0, - "entitiesUpdated": 1 + "entitiesUpdated": 0, + "slugConflicts": [ + "o-1" + ] }, "trace": [ [ @@ -940,84 +940,12 @@ "gemeente-g-zn" ], [ - "log.info", - "OrganizationSyncService: Found existing entity by slug, updating UUID to match object", + "log.warning", + "OrganizationSyncService: an organisation with this name already exists; the organisatie is left unlinked for an administrator", { "app": "stackiq", "organisatieId": "o-1", - "oldEntityUuid": "o-old", - "slug": "gemeente-g-zn" - } - ], - [ - "entity.setUuid", - "o-old", - "o-1" - ], - [ - "mapper.save", - "o-1", - true - ], - [ - "log.info", - "OrganizationSyncService: Updating organisatie object owner and organisation", - { - "organisatieId": "o-1", - "organisationEntityUuid": "o-1", - "register": "reg-v", - "schema": "org-schema" - } - ], - [ - "entity.setObject", - "o-1", - { - "name": "Gemeente G & Zn!", - "status": "actief", - "organisation": "o-1", - "@self": { - "owner": "o-1", - "organisation": "o-1" - } - } - ], - [ - "entity.setOwner", - "o-1", - "o-1" - ], - [ - "entity.setOrganisation", - "o-1", - "o-1" - ], - [ - "objects.saveObject", - { - "object": { - "name": "Gemeente G & Zn!", - "status": "actief", - "organisation": "o-1", - "@self": { - "organisation": "o-1" - } - }, - "register": "reg-entity", - "schema": "schema-entity", - "uuid": "o-1", - "silent": true, - "_validation": false - } - ], - [ - "log.info", - "OrganizationSyncService: Successfully updated organisatie object owner and organisation", - { - "organisatieId": "o-1", - "organisationEntityUuid": "o-1", - "ownerSet": "o-1", - "organisationSet": "o-1" + "existingOrganisationUuid": "o-old" } ] ] @@ -1848,5 +1776,471 @@ } ] ] + }, + "entity: same name as another organisation": { + "returned": null, + "stats": { + "entitiesCreated": 0, + "entitiesUpdated": 0, + "slugConflicts": [ + "o-1" + ] + }, + "trace": [ + [ + "objects.find", + { + "id": "o-1", + "register": "reg-entity", + "schema": "schema-entity", + "_rbac": false, + "_multitenancy": false + } + ], + [ + "log.debug", + "Ensuring organisation entity", + { + "app": "stackiq", + "organisatieId": "o-1", + "name": "Gemeente A", + "status": "actief" + } + ], + [ + "settings.getVoorzieningenConfig" + ], + [ + "mapper.findByUuid", + "o-1" + ], + [ + "mapper.findBySlug", + "gemeente-a" + ], + [ + "log.warning", + "OrganizationSyncService: an organisation with this name already exists; the organisatie is left unlinked for an administrator", + { + "app": "stackiq", + "organisatieId": "o-1", + "existingOrganisationUuid": "o-other" + } + ] + ] + }, + "entity: long name, slug cut at 100": { + "returned": null, + "stats": { + "entitiesCreated": 0, + "entitiesUpdated": 0, + "slugConflicts": [ + "o-1" + ] + }, + "trace": [ + [ + "objects.find", + { + "id": "o-1", + "register": "reg-entity", + "schema": "schema-entity", + "_rbac": false, + "_multitenancy": false + } + ], + [ + "log.debug", + "Ensuring organisation entity", + { + "app": "stackiq", + "organisatieId": "o-1", + "name": "Gemeente aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "status": "actief" + } + ], + [ + "settings.getVoorzieningenConfig" + ], + [ + "mapper.findByUuid", + "o-1" + ], + [ + "mapper.findBySlug", + "gemeente-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + ], + [ + "log.warning", + "OrganizationSyncService: an organisation with this name already exists; the organisatie is left unlinked for an administrator", + { + "app": "stackiq", + "organisatieId": "o-1", + "existingOrganisationUuid": "o-other" + } + ] + ] + }, + "entity: long name, slug free": { + "returned": [ + "new-entity", + true + ], + "stats": { + "entitiesCreated": 1, + "entitiesUpdated": 0 + }, + "trace": [ + [ + "objects.find", + { + "id": "o-1", + "register": "reg-entity", + "schema": "schema-entity", + "_rbac": false, + "_multitenancy": false + } + ], + [ + "log.debug", + "Ensuring organisation entity", + { + "app": "stackiq", + "organisatieId": "o-1", + "name": "Gemeente aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "status": "actief" + } + ], + [ + "settings.getVoorzieningenConfig" + ], + [ + "mapper.findByUuid", + "o-1" + ], + [ + "mapper.findBySlug", + "gemeente-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + ], + [ + "log.debug", + "Creating new organisation entity", + { + "app": "stackiq", + "organisatieId": "o-1", + "name": "Gemeente aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + } + ], + [ + "organisations.create", + { + "name": "Gemeente aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "status": "actief" + } + ], + [ + "log.debug", + "New organisation entity created", + { + "app": "stackiq", + "organisatieId": "o-1", + "entityId": 41, + "active": true, + "name": "Entity new-entity" + } + ], + [ + "log.info", + "[FLOW] Sending organization registration email", + { + "organisatieId": "o-1" + } + ], + [ + "mail.sendOrganizationRegistrationEmail", + "Gemeente aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + ], + [ + "log.info", + "📧 Organization registration email sent successfully", + { + "organisatieId": "o-1" + } + ], + [ + "log.info", + "OrganizationSyncService: Updating organisatie object owner and organisation", + { + "organisatieId": "o-1", + "organisationEntityUuid": "new-entity", + "register": "reg-v", + "schema": "org-schema" + } + ], + [ + "entity.setObject", + "o-1", + { + "name": "Gemeente aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "status": "actief", + "organisation": "new-entity", + "@self": { + "owner": "new-entity", + "organisation": "new-entity" + } + } + ], + [ + "entity.setOwner", + "o-1", + "new-entity" + ], + [ + "entity.setOrganisation", + "o-1", + "new-entity" + ], + [ + "objects.saveObject", + { + "object": { + "name": "Gemeente aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "status": "actief", + "organisation": "new-entity", + "@self": { + "organisation": "new-entity" + } + }, + "register": "reg-entity", + "schema": "schema-entity", + "uuid": "o-1", + "silent": true, + "_validation": false + } + ], + [ + "log.info", + "OrganizationSyncService: Successfully updated organisatie object owner and organisation", + { + "organisatieId": "o-1", + "organisationEntityUuid": "new-entity", + "ownerSet": "new-entity", + "organisationSet": "new-entity" + } + ] + ] + }, + "entity: OpenRegister refuses the slug": { + "returned": null, + "stats": { + "entitiesCreated": 0, + "entitiesUpdated": 0, + "slugConflicts": [ + "o-1" + ] + }, + "trace": [ + [ + "objects.find", + { + "id": "o-1", + "register": "reg-entity", + "schema": "schema-entity", + "_rbac": false, + "_multitenancy": false + } + ], + [ + "log.debug", + "Ensuring organisation entity", + { + "app": "stackiq", + "organisatieId": "o-1", + "name": "Gemeente P", + "status": "actief" + } + ], + [ + "settings.getVoorzieningenConfig" + ], + [ + "mapper.findByUuid", + "o-1" + ], + [ + "mapper.findBySlug", + "gemeente-p" + ], + [ + "log.debug", + "Creating new organisation entity", + { + "app": "stackiq", + "organisatieId": "o-1", + "name": "Gemeente P" + } + ], + [ + "organisations.create", + { + "name": "Gemeente P", + "status": "actief" + } + ], + [ + "log.warning", + "OrganizationSyncService: an organisation with this name already exists; the organisatie is left unlinked for an administrator", + { + "app": "stackiq", + "organisatieId": "o-1", + "existingOrganisationUuid": null + } + ] + ] + }, + "entity: named Unknown": { + "returned": [ + "new-entity", + true + ], + "stats": { + "entitiesCreated": 1, + "entitiesUpdated": 0 + }, + "trace": [ + [ + "objects.find", + { + "id": "o-1", + "register": "reg-entity", + "schema": "schema-entity", + "_rbac": false, + "_multitenancy": false + } + ], + [ + "log.debug", + "Ensuring organisation entity", + { + "app": "stackiq", + "organisatieId": "o-1", + "name": "Unknown", + "status": "actief" + } + ], + [ + "settings.getVoorzieningenConfig" + ], + [ + "mapper.findByUuid", + "o-1" + ], + [ + "log.debug", + "Creating new organisation entity", + { + "app": "stackiq", + "organisatieId": "o-1", + "name": "Unknown" + } + ], + [ + "organisations.create", + { + "name": "Unknown", + "status": "actief" + } + ], + [ + "log.debug", + "New organisation entity created", + { + "app": "stackiq", + "organisatieId": "o-1", + "entityId": 41, + "active": true, + "name": "Entity new-entity" + } + ], + [ + "log.info", + "[FLOW] Sending organization registration email", + { + "organisatieId": "o-1" + } + ], + [ + "mail.sendOrganizationRegistrationEmail", + "Unknown" + ], + [ + "log.info", + "📧 Organization registration email sent successfully", + { + "organisatieId": "o-1" + } + ], + [ + "log.info", + "OrganizationSyncService: Updating organisatie object owner and organisation", + { + "organisatieId": "o-1", + "organisationEntityUuid": "new-entity", + "register": "reg-v", + "schema": "org-schema" + } + ], + [ + "entity.setObject", + "o-1", + { + "name": "Unknown", + "status": "actief", + "organisation": "new-entity", + "@self": { + "owner": "new-entity", + "organisation": "new-entity" + } + } + ], + [ + "entity.setOwner", + "o-1", + "new-entity" + ], + [ + "entity.setOrganisation", + "o-1", + "new-entity" + ], + [ + "objects.saveObject", + { + "object": { + "name": "Unknown", + "status": "actief", + "organisation": "new-entity", + "@self": { + "organisation": "new-entity" + } + }, + "register": "reg-entity", + "schema": "schema-entity", + "uuid": "o-1", + "silent": true, + "_validation": false + } + ], + [ + "log.info", + "OrganizationSyncService: Successfully updated organisatie object owner and organisation", + { + "organisatieId": "o-1", + "organisationEntityUuid": "new-entity", + "ownerSet": "new-entity", + "organisationSet": "new-entity" + } + ] + ] } } From 04a73f792230d93957e3b45e1496b36a75df34a4 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Tue, 3 Nov 2026 21:55:50 +0100 Subject: [PATCH 2/3] fix(organisations): check the slug of the name OpenRegister receives The sync checked the slug of the organisatie's own name and skipped an empty or "Unknown" one, while OpenRegister receives "Organisation " for an empty, "Unknown" or "0" name. That name reached OpenRegister's create with no check. OrganisationSlug::nameFor() now gives the name both the check and the create use. A contact person whose organisation has no entity (refused slug, missing object) no longer ends the event handler or the batch sync with an Error on getActive(): both paths go through organisationIsActive(). occ stackiq:organisation:link refuses OpenRegister's default organisation, names what keeps the old uuid, and links only after confirmation or --yes. OrganisatieService drops two collaborators it never used. Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/AppInfo/Application.php | 4 - lib/Command/OrganisationLinkCommand.php | 59 ++++++- lib/Service/OrganisatieService.php | 19 +-- lib/Service/OrganisationSlug.php | 38 ++++- lib/Service/OrganizationSyncService.php | 115 ++++++++++---- tests/Stubs/Service/OrganisationService.php | 7 + .../Command/OrganisationLinkCommandTest.php | 146 ++++++++++++++---- .../OrganisatieServiceSlugConflictTest.php | 40 ++++- tests/Unit/Service/OrganisationSlugTest.php | 94 +++++++++++ ...rganizationSyncContactPersonGoldenTest.php | 56 +++++++ .../OrganizationSyncEntityGoldenTest.php | 101 +++++++++++- .../golden/organizationSyncContactPerson.json | 30 +++- .../golden/organizationSyncEntity.json | 8 + 13 files changed, 626 insertions(+), 91 deletions(-) create mode 100644 tests/Unit/Service/OrganisationSlugTest.php diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 7ff02596d..0686f541a 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -264,13 +264,9 @@ private function registerDomainServices(IRegistrationContext $context): void { OrganisatieService::class, function ($container) { return new OrganisatieService( - organizationHandler: $container->get( - OrganizationHandler::class - ), logger: $container->get('Psr\Log\LoggerInterface'), container: $container, appManager: $container->get('OCP\App\IAppManager'), - config: $container->get(IAppConfig::class), userManager: $container->get(IUserManager::class), emailService: $container->get(SymfonyEmailService::class), ); diff --git a/lib/Command/OrganisationLinkCommand.php b/lib/Command/OrganisationLinkCommand.php index bf97111da..610df6a82 100644 --- a/lib/Command/OrganisationLinkCommand.php +++ b/lib/Command/OrganisationLinkCommand.php @@ -26,9 +26,12 @@ use OCA\Stackiq\Service\OrganizationSyncService; use Psr\Container\ContainerInterface; use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Helper\QuestionHelper; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; +use Symfony\Component\Console\Question\ConfirmationQuestion; use Throwable; /** @@ -37,6 +40,8 @@ * The organisation sync never takes over an OpenRegister organisation whose * name gives the same slug: it leaves the organisatie unlinked and logs it. * This command is how an administrator links the two when they are the same. + * The organisation takes the organisatie's uuid, so the command first says what + * keeps the old uuid and asks for confirmation (or --yes). * * @spec openspec/specs/organisatie-service/spec.md * @@ -53,6 +58,18 @@ class OrganisationLinkCommand extends Command { 'already-linked' => 'This organisatie already has an OpenRegister organisation.', 'no-organisation' => 'No OpenRegister organisation has this uuid.', 'organisation-in-use' => 'This OpenRegister organisation belongs to another organisatie.', + 'default-organisation' => 'This is the OpenRegister default organisation, which is never linked.', + 'not-confirmed' => 'Not linked. Confirm, or run again with --yes.', + ]; + + /** + * What still holds the organisation's old uuid after a link. + */ + private const STORED_REFERENCES = [ + 'objects in OpenRegister stamped with this organisation (their @self.organisation)', + 'the active organisation of its members', + 'the parent of its child organisations', + 'references in other apps', ]; /** @@ -77,7 +94,8 @@ protected function configure(): void { $this->setName('stackiq:organisation:link') ->setDescription('Link an organisatie to an existing OpenRegister organisation.') ->addArgument('organisatie', InputArgument::REQUIRED, 'The organisatie object uuid.') - ->addArgument('organisation', InputArgument::REQUIRED, 'The OpenRegister organisation uuid.'); + ->addArgument('organisation', InputArgument::REQUIRED, 'The OpenRegister organisation uuid.') + ->addOption('yes', 'y', InputOption::VALUE_NONE, 'Link without asking for confirmation.'); }//end configure() /** @@ -91,11 +109,18 @@ protected function configure(): void { * @spec openspec/specs/organisatie-service/spec.md */ protected function execute(InputInterface $input, OutputInterface $output): int { + $organisatie = (string) $input->getArgument('organisatie'); + $organisation = (string) $input->getArgument('organisation'); try { - $outcome = $this->container->get(OrganizationSyncService::class)->linkOrganisationEntity( - organisatieUuid: (string) $input->getArgument('organisatie'), - entityUuid: (string) $input->getArgument('organisation') - ); + $sync = $this->container->get(OrganizationSyncService::class); + $outcome = $sync->checkOrganisationLink(organisatieUuid: $organisatie, entityUuid: $organisation); + if ($outcome === null && $this->confirmed(input: $input, output: $output) === false) { + $outcome = 'not-confirmed'; + } + + if ($outcome === null) { + $outcome = $sync->linkOrganisationEntity(organisatieUuid: $organisatie, entityUuid: $organisation); + } } catch (Throwable $e) { $output->writeln(messages: 'Could not link: ' . $e->getMessage() . ''); return 1; @@ -108,4 +133,28 @@ protected function execute(InputInterface $input, OutputInterface $output): int return 1; }//end execute() + + /** + * Say what keeps the organisation's old uuid, and ask whether to link anyway. + * + * @param InputInterface $input Console input. + * @param OutputInterface $output Console output. + * + * @return bool True when --yes is given or the administrator confirms. + */ + private function confirmed(InputInterface $input, OutputInterface $output): bool { + $organisation = (string) $input->getArgument('organisation'); + $organisatie = (string) $input->getArgument('organisatie'); + $output->writeln(messages: 'The OpenRegister organisation ' . $organisation . ' will take the uuid ' . $organisatie . '.'); + $output->writeln(messages: 'These keep the old uuid and are not moved:'); + foreach (self::STORED_REFERENCES as $reference) { + $output->writeln(messages: ' - ' . $reference); + } + + if ($input->getOption('yes') === true) { + return true; + } + + return (new QuestionHelper())->ask($input, $output, new ConfirmationQuestion('Link anyway? [y/N] ', false)) === true; + }//end confirmed() }//end class diff --git a/lib/Service/OrganisatieService.php b/lib/Service/OrganisatieService.php index b76a4a36a..72f41e233 100644 --- a/lib/Service/OrganisatieService.php +++ b/lib/Service/OrganisatieService.php @@ -19,9 +19,7 @@ namespace OCA\Stackiq\Service; -use OCA\Stackiq\Service\Stackiq\OrganizationHandler; use OCP\App\IAppManager; -use OCP\IAppConfig; use OCP\IUserManager; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; @@ -40,6 +38,8 @@ * @link https://github.com/ConductionNL/stackiq * * @SuppressWarnings(PHPMD.UnusedPrivateMethod) + * + * @spec openspec/specs/organisatie-service/spec.md */ class OrganisatieService { /** @@ -51,20 +51,16 @@ class OrganisatieService { /** * OrganisatieService constructor. * - * @param OrganizationHandler $organizationHandler Organization handler * @param LoggerInterface $logger Logger interface * @param ContainerInterface $container Container interface * @param IAppManager $appManager App manager - * @param IAppConfig $config Configuration service * @param IUserManager $userManager User manager service * @param SymfonyEmailService $emailService Email service */ public function __construct( - private readonly OrganizationHandler $organizationHandler, private readonly LoggerInterface $logger, private readonly ContainerInterface $container, private readonly IAppManager $appManager, - private readonly IAppConfig $config, private readonly IUserManager $userManager, private readonly SymfonyEmailService $emailService, ) { @@ -222,17 +218,8 @@ private function getOrganisationService(): ?\OCA\OpenRegister\Service\Organisati * @spec openspec/specs/organisatie-service/spec.md */ private function mapOrganizationDataForOpenRegister(array $objectData): array { - // Get the organization name - try 'name' first, then 'name', then use UUID as fallback. - $name = $objectData['name'] ?? null; - - // If still no name, create a unique one using the ID to avoid slug conflicts. - if (empty($name) === true || $name === 'Unknown') { - $orgId = $objectData['id'] ?? uniqid(prefix: 'org-'); - $name = 'Organisation ' . substr($orgId, 0, 8); - } - return [ - 'name' => $name, + 'name' => (new OrganisationSlug())->nameFor(objectData: $objectData), 'type' => $objectData['type'] ?? '', 'website' => $objectData['website'] ?? '', 'active' => $this->mapStatus(status: $objectData['status'] ?? $objectData['beoordeling'] ?? 'actief'), diff --git a/lib/Service/OrganisationSlug.php b/lib/Service/OrganisationSlug.php index 52cd6ee2e..27a91e1fa 100644 --- a/lib/Service/OrganisationSlug.php +++ b/lib/Service/OrganisationSlug.php @@ -22,7 +22,9 @@ /** * Mirrors OpenRegister's OrganisationService::generateSlug(): lower case, runs of - * other characters as one hyphen, no hyphen at either end, at most 100 characters. + * other characters as one hyphen, no hyphen at either end, at most 100 characters + * (cut after the trim, so a hyphen at position 100 stays). It also gives the name + * stackiq sends OpenRegister for an organisatie. * It also names OpenRegister's exception for a taken slug. * * @spec openspec/specs/organisatie-service/spec.md @@ -53,4 +55,38 @@ public function fromName(string $name): string { $slug = trim((string) preg_replace('/[^a-z0-9]+/', '-', strtolower($name)), '-'); return substr($slug, 0, self::MAX_LENGTH); }//end fromName() + + /** + * The name OpenRegister receives for an organisatie. + * + * An empty, "Unknown" or "0" name is replaced by "Organisation " and the first + * 8 characters of the organisatie id, so the slug to check is that name's. + * + * @param array $objectData The organisatie object data. + * + * @return string The organisation name. + * + * @spec openspec/specs/organisatie-service/spec.md + */ + public function nameFor(array $objectData): string { + $name = ($objectData['name'] ?? null); + if (empty($name) === true || $name === 'Unknown') { + return 'Organisation ' . substr((string) ($objectData['id'] ?? ''), 0, 8); + } + + return (string) $name; + }//end nameFor() + + /** + * The slug OpenRegister gives the organisation of an organisatie. + * + * @param array $objectData The organisatie object data. + * + * @return string The slug. + * + * @spec openspec/specs/organisatie-service/spec.md + */ + public function fromObjectData(array $objectData): string { + return $this->fromName(name: $this->nameFor(objectData: $objectData)); + }//end fromObjectData() }//end class diff --git a/lib/Service/OrganizationSyncService.php b/lib/Service/OrganizationSyncService.php index 7a3a6332c..d1a0325cd 100644 --- a/lib/Service/OrganizationSyncService.php +++ b/lib/Service/OrganizationSyncService.php @@ -1107,28 +1107,18 @@ private function refreshOrganisationEntity(object $organisationEntity, array $ru * Link an organisatie to an existing OpenRegister organisation, as an administrator decides. * * The organisation takes the organisatie's uuid, which is what links the two, - * and becomes the organisatie object's owner. Refused when the organisatie is - * unknown or already has an organisation, when the organisation is unknown, or - * when the organisation is already another organisatie's. + * and becomes the organisatie object's owner. Refused for the reasons + * checkOrganisationLink() gives. * * @param string $organisatieUuid The organisatie object's uuid. * @param string $entityUuid The OpenRegister organisation's uuid. * - * @return string linked, no-organisatie, already-linked, no-organisation or organisation-in-use. + * @return string linked, or the refusal checkOrganisationLink() gives. * * @spec openspec/specs/organisatie-service/spec.md */ public function linkOrganisationEntity(string $organisatieUuid, string $entityUuid): string { - $config = $this->settingsService->getVoorzieningenConfig(); - $run = [ - 'register' => (string) ($config['register'] ?? ''), - 'organizationSchema' => (string) ($config['organisatie_schema'] ?? ''), - ]; - $run['organisationObject'] = $this->findOrganisatie(run: $run, uuid: $organisatieUuid); - if ($run['organisationObject'] === null) { - return 'no-organisatie'; - } - + $run = $this->linkRun(organisatieUuid: $organisatieUuid); $outcome = $this->linkRefusal(run: $run, uuids: [$organisatieUuid, $entityUuid]); if ($outcome !== null) { return $outcome; @@ -1146,30 +1136,89 @@ public function linkOrganisationEntity(string $organisatieUuid, string $entityUu return 'linked'; }//end linkOrganisationEntity() + /** + * Why an organisatie may not be linked to an OpenRegister organisation, or null when it may. + * + * Refused when the organisatie is unknown or already has an organisation, when + * the organisation is unknown, is OpenRegister's default organisation, or is + * already another organisatie's. Nothing is written. + * + * @param string $organisatieUuid The organisatie object's uuid. + * @param string $entityUuid The OpenRegister organisation's uuid. + * + * @return string|null no-organisatie, already-linked, no-organisation, + * default-organisation, organisation-in-use, or null. + * + * @spec openspec/specs/organisatie-service/spec.md + */ + public function checkOrganisationLink(string $organisatieUuid, string $entityUuid): ?string { + return $this->linkRefusal( + run: $this->linkRun(organisatieUuid: $organisatieUuid), + uuids: [$organisatieUuid, $entityUuid] + ); + }//end checkOrganisationLink() + + /** + * The register, organisatie schema and organisatie object a link works on. + * + * @param string $organisatieUuid The organisatie object's uuid. + * + * @return array The run: register, organizationSchema and organisationObject (null when unknown). + */ + private function linkRun(string $organisatieUuid): array { + $config = $this->settingsService->getVoorzieningenConfig(); + $run = [ + 'register' => (string) ($config['register'] ?? ''), + 'organizationSchema' => (string) ($config['organisatie_schema'] ?? ''), + ]; + $run['organisationObject'] = $this->findOrganisatie(run: $run, uuid: $organisatieUuid); + return $run; + }//end linkRun() + /** * Why a link may not be made, or null when it may. * - * @param array $run The register and organisatie schema. + * @param array $run The link run (see linkRun()). * @param array{0: string, 1: string} $uuids The organisatie uuid and the organisation uuid. * - * @return string|null already-linked, no-organisation, organisation-in-use, or null. + * @return string|null The refusal, or null. */ private function linkRefusal(array $run, array $uuids): ?string { [$organisatieUuid, $entityUuid] = $uuids; + if ($run['organisationObject'] === null) { + return 'no-organisatie'; + } + if ($this->organisationExists(uuid: $organisatieUuid) === true) { return 'already-linked'; } + return $this->organisationRefusal(run: $run, entityUuid: $entityUuid); + }//end linkRefusal() + + /** + * Why this OpenRegister organisation may not take an organisatie's uuid, or null when it may. + * + * @param array $run The link run (see linkRun()). + * @param string $entityUuid The OpenRegister organisation's uuid. + * + * @return string|null no-organisation, default-organisation, organisation-in-use, or null. + */ + private function organisationRefusal(array $run, string $entityUuid): ?string { if ($this->organisationExists(uuid: $entityUuid) === false) { return 'no-organisation'; } + if ($this->container->get('OCA\OpenRegister\Service\OrganisationService')->getDefaultOrganisationUuid() === $entityUuid) { + return 'default-organisation'; + } + if ($this->findOrganisatie(run: $run, uuid: $entityUuid) !== null) { return 'organisation-in-use'; } return null; - }//end linkRefusal() + }//end organisationRefusal() /** * Whether an OpenRegister organisation has this uuid. @@ -1210,24 +1259,19 @@ private function findOrganisatie(array $run, string $uuid): ?object { }//end findOrganisatie() /** - * Whether another OpenRegister organisation already has the slug this organisatie's name gives. + * Whether another OpenRegister organisation already has the slug of the name + * OpenRegister would receive for this organisatie (OrganisationSlug::nameFor()). * * Such an organisation is never taken over on a name match: the organisatie is * left without an entity, logged and counted, for an administrator to link - * explicitly (occ stackiq:organisation:link). A missing or "Unknown" name gets a - * uuid-based name when the entity is created, so it is not checked. + * explicitly (occ stackiq:organisation:link). * * @param array $run The ensure run (see ensureOrganisationEntity). * * @return bool True when the slug is taken and nothing may be created. */ private function slugIsTaken(array $run): bool { - $orgName = (string) ($run['objectData']['name'] ?? ''); - if ($orgName === '' || $orgName === 'Unknown') { - return false; - } - - $slug = (new OrganisationSlug())->fromName(name: $orgName); + $slug = (new OrganisationSlug())->fromObjectData(objectData: $run['objectData']); try { $existing = $this->organisationMapper->findBySlug($slug); } catch (\OCP\AppFramework\Db\DoesNotExistException $e) { @@ -2707,11 +2751,11 @@ private function provisionContactUser(array $run): void { ); try { - if ($organisationEntity !== false && $organisationEntity->getActive() === true) { + if ($this->organisationIsActive(organisationEntity: $organisationEntity) === true) { $this->createUserForContact(run: $run); } - if ($organisationEntity === false || $organisationEntity->getActive() !== true) { + if ($this->organisationIsActive(organisationEntity: $organisationEntity) === false) { $this->logSkippedUserCreation(organisationEntity: $organisationEntity, run: $run); } } catch (\Exception $e) { @@ -2729,6 +2773,17 @@ private function provisionContactUser(array $run): void { }//end provisionContactUser() + /** + * Whether a contact person's organisation entity exists and is active. + * + * @param mixed $organisationEntity The organisation entity, or null when there is none. + * + * @return bool + */ + private function organisationIsActive(mixed $organisationEntity): bool { + return $organisationEntity !== null && $organisationEntity->getActive() === true; + }//end organisationIsActive() + /** * The organisation entity of a contact person's organisation, created from * the organisation object when the entity is missing. @@ -3085,7 +3140,7 @@ public function processSpecificContactPerson($contactObject): array { }//end try try { - if ($organisationEntity !== false && $organisationEntity->getActive() === true) { + if ($this->organisationIsActive(organisationEntity: $organisationEntity) === true) { // Determine if this is the first contact for the organization. $isFirstContact = $this->contactpersonHandler->isFirstContactForOrganization( $contactObject, @@ -3168,7 +3223,7 @@ public function processSpecificContactPerson($contactObject): array { } }//end if - if ($organisationEntity === false || $organisationEntity->getActive() !== true) { + if ($this->organisationIsActive(organisationEntity: $organisationEntity) === false) { if ($organisationEntity !== null) { $organizationActiveValue = $organisationEntity->getActive(); } else { diff --git a/tests/Stubs/Service/OrganisationService.php b/tests/Stubs/Service/OrganisationService.php index bb1695b52..6000f7527 100644 --- a/tests/Stubs/Service/OrganisationService.php +++ b/tests/Stubs/Service/OrganisationService.php @@ -46,6 +46,13 @@ abstract public function getActiveOrganisation(): ?Organisation; */ abstract public function ensureDefaultOrganisation(): Organisation; + /** + * The uuid of the instance's default organisation, or null when none is set. + * + * @return string|null + */ + abstract public function getDefaultOrganisationUuid(): ?string; + /** * Get the caller's own organisations (session-scoped). * diff --git a/tests/Unit/Command/OrganisationLinkCommandTest.php b/tests/Unit/Command/OrganisationLinkCommandTest.php index 42fe7601e..972e50cd4 100644 --- a/tests/Unit/Command/OrganisationLinkCommandTest.php +++ b/tests/Unit/Command/OrganisationLinkCommandTest.php @@ -25,7 +25,8 @@ use Symfony\Component\Console\Tester\CommandTester; /** - * The command passes both uuids on and reports the outcome. + * The command checks the link, says what keeps the old uuid, asks for + * confirmation, and reports the outcome. * * @covers \OCA\Stackiq\Command\OrganisationLinkCommand * @@ -34,48 +35,139 @@ class OrganisationLinkCommandTest extends TestCase { /** - * Each outcome, its exit code and what the console says. + * Each refusal the check gives, and what the console says. * - * @return array + * @return array */ - public static function outcomes(): array { + public static function refusals(): array { return [ - 'linked' => ['linked', 0, 'Linked.'], - 'no organisatie' => ['no-organisatie', 1, 'No organisatie has this uuid.'], - 'already linked' => ['already-linked', 1, 'already has an OpenRegister organisation'], - 'no organisation' => ['no-organisation', 1, 'No OpenRegister organisation has this uuid.'], - 'organisation in use' => ['organisation-in-use', 1, 'belongs to another organisatie'], - 'failure' => [new RuntimeException('db down'), 1, 'Could not link: db down'], + 'no organisatie' => ['no-organisatie', 'No organisatie has this uuid.'], + 'already linked' => ['already-linked', 'already has an OpenRegister organisation'], + 'no organisation' => ['no-organisation', 'No OpenRegister organisation has this uuid.'], + 'organisation in use' => ['organisation-in-use', 'belongs to another organisatie'], + 'default organisation' => ['default-organisation', 'default organisation, which is never linked'], + 'failure' => [new RuntimeException('db down'), 'Could not link: db down'], ]; - }//end outcomes() + }//end refusals() /** - * The outcome is reported with its exit code, for the uuids given. + * A refused link exits 1, asks nothing and never links. * - * @param string|RuntimeException $outcome What the sync service answers. - * @param int $code The exit code. + * @param string|RuntimeException $refusal What the check answers. * @param string $text Text the console shows. * * @return void * - * @dataProvider outcomes + * @dataProvider refusals */ - public function testTheOutcomeIsReported(string|RuntimeException $outcome, int $code, string $text): void { - $sync = $this->createMock(OrganizationSyncService::class); - $call = $sync->expects(self::once())->method('linkOrganisationEntity')->with('org-uuid', 'or-uuid'); - if ($outcome instanceof RuntimeException) { - $call->willThrowException($outcome); + public function testARefusedLinkIsNeverMade(string|RuntimeException $refusal, string $text): void { + $sync = $this->createMock(OrganizationSyncService::class); + $check = $sync->expects(self::once())->method('checkOrganisationLink')->with('org-uuid', 'or-uuid'); + if ($refusal instanceof RuntimeException) { + $check->willThrowException($refusal); } else { - $call->willReturn($outcome); + $check->willReturn($refusal); } - $container = $this->createMock(ContainerInterface::class); - $container->method('get')->with(OrganizationSyncService::class)->willReturn($sync); + $sync->expects(self::never())->method('linkOrganisationEntity'); - $tester = new CommandTester(new OrganisationLinkCommand($container)); - $tester->execute(['organisatie' => 'org-uuid', 'organisation' => 'or-uuid']); + $tester = $this->tester(sync: $sync); + $tester->execute(['organisatie' => 'org-uuid', 'organisation' => 'or-uuid', '--yes' => true]); - self::assertSame($code, $tester->getStatusCode()); + self::assertSame(1, $tester->getStatusCode()); self::assertStringContainsString($text, $tester->getDisplay()); - }//end testTheOutcomeIsReported() + self::assertStringNotContainsString('keep the old uuid', $tester->getDisplay()); + }//end testARefusedLinkIsNeverMade() + + /** + * How the administrator answers, and whether the link is made. + * + * @return array, 2: bool, 3: string}> + */ + public static function answers(): array { + return [ + '--yes' => [['--yes' => true], [], true, 'Linked.'], + 'confirmed' => [[], ['y'], true, 'Linked.'], + 'declined' => [[], ['n'], false, 'Not linked.'], + 'no answer' => [[], [''], false, 'Not linked.'], + ]; + }//end answers() + + /** + * An allowed link names what keeps the old uuid and is made only once confirmed. + * + * @param array $options The command options. + * @param array $inputs What the administrator types. + * @param bool $linked Whether the link is made. + * @param string $text Text the console shows. + * + * @return void + * + * @dataProvider answers + */ + public function testAnAllowedLinkWarnsAndAsks(array $options, array $inputs, bool $linked, string $text): void { + $sync = $this->createMock(OrganizationSyncService::class); + $sync->method('checkOrganisationLink')->with('org-uuid', 'or-uuid')->willReturn(null); + $link = $sync->expects($linked === true ? self::once() : self::never())->method('linkOrganisationEntity'); + $link->with('org-uuid', 'or-uuid')->willReturn('linked'); + + $tester = $this->tester(sync: $sync); + $tester->setInputs($inputs); + $tester->execute(array_merge(['organisatie' => 'org-uuid', 'organisation' => 'or-uuid'], $options)); + + $display = $tester->getDisplay(); + self::assertSame($linked === true ? 0 : 1, $tester->getStatusCode()); + self::assertStringContainsString($text, $display); + self::assertStringContainsString('The OpenRegister organisation or-uuid will take the uuid org-uuid.', $display); + foreach (['@self.organisation', 'active organisation of its members', 'parent of its child organisations', 'other apps'] as $reference) { + self::assertStringContainsString($reference, $display); + } + }//end testAnAllowedLinkWarnsAndAsks() + + /** + * Without a terminal and without --yes nothing is linked. + * + * @return void + */ + public function testANonInteractiveRunNeedsYes(): void { + $sync = $this->createMock(OrganizationSyncService::class); + $sync->method('checkOrganisationLink')->willReturn(null); + $sync->expects(self::never())->method('linkOrganisationEntity'); + + $tester = $this->tester(sync: $sync); + $tester->execute(['organisatie' => 'org-uuid', 'organisation' => 'or-uuid'], ['interactive' => false]); + + self::assertSame(1, $tester->getStatusCode()); + self::assertStringContainsString('run again with --yes', $tester->getDisplay()); + }//end testANonInteractiveRunNeedsYes() + + /** + * A failing link after confirmation exits 1. + * + * @return void + */ + public function testAFailingLinkIsReported(): void { + $sync = $this->createMock(OrganizationSyncService::class); + $sync->method('checkOrganisationLink')->willReturn(null); + $sync->method('linkOrganisationEntity')->willThrowException(new RuntimeException('db down')); + + $tester = $this->tester(sync: $sync); + $tester->execute(['organisatie' => 'org-uuid', 'organisation' => 'or-uuid', '--yes' => true]); + + self::assertSame(1, $tester->getStatusCode()); + self::assertStringContainsString('Could not link: db down', $tester->getDisplay()); + }//end testAFailingLinkIsReported() + + /** + * The command over a container that answers the sync service. + * + * @param OrganizationSyncService $sync The sync service double. + * + * @return CommandTester + */ + private function tester(OrganizationSyncService $sync): CommandTester { + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->with(OrganizationSyncService::class)->willReturn($sync); + return new CommandTester(new OrganisationLinkCommand($container)); + }//end tester() }//end class diff --git a/tests/Unit/Service/OrganisatieServiceSlugConflictTest.php b/tests/Unit/Service/OrganisatieServiceSlugConflictTest.php index f0b9eeb55..253f43c70 100644 --- a/tests/Unit/Service/OrganisatieServiceSlugConflictTest.php +++ b/tests/Unit/Service/OrganisatieServiceSlugConflictTest.php @@ -17,8 +17,10 @@ namespace OCA\Stackiq\Tests\Unit\Service; +use OCA\OpenRegister\Db\Organisation; use OCA\OpenRegister\Service\OrganisationService; use OCA\Stackiq\Service\OrganisatieService; +use OCA\Stackiq\Service\OrganisationSlug; use OCP\App\IAppManager; use PHPUnit\Framework\TestCase; use Psr\Container\ContainerInterface; @@ -33,6 +35,7 @@ * A taken slug reaches the caller; any other failure still ends as null. * * @covers \OCA\Stackiq\Service\OrganisatieService + * @uses \OCA\Stackiq\Service\OrganisationSlug * * @spec openspec/specs/organisatie-service/spec.md */ @@ -61,17 +64,44 @@ public function testAnotherFailureEndsAsNull(): void { ); }//end testAnotherFailureEndsAsNull() + /** + * OpenRegister receives the name OrganisationSlug::nameFor() gives, which is + * the name the sync checks the slug of. + * + * @return void + */ + public function testOpenRegisterReceivesTheNameWhoseSlugIsChecked(): void { + $id = 'abcd1234-5678-4abc-8def-0123456789ab'; + foreach (['Unknown', '0', '', null, 'Gemeente A'] as $name) { + $data = ['id' => $id]; + if ($name !== null) { + $data['name'] = $name; + } + + $organisations = $this->createMock(OrganisationService::class); + $organisations->method('getActiveOrganisation')->willReturn(null); + $organisations->expects(self::once())->method('createOrganisation') + ->with((new OrganisationSlug())->nameFor(objectData: $data), self::anything(), false, $id) + ->willReturn($this->createMock(Organisation::class)); + + $this->service(organisations: $organisations)->createOrganisationInOpenRegister($data); + } + }//end testOpenRegisterReceivesTheNameWhoseSlugIsChecked() + /** * The service over an OpenRegister whose createOrganisation() throws. * - * @param \Throwable $failure What createOrganisation() throws. + * @param \Throwable|null $failure What createOrganisation() throws. + * @param OrganisationService|null $organisations OpenRegister's service, when not one that throws. * * @return OrganisatieService */ - private function service(\Throwable $failure): OrganisatieService { - $organisations = $this->createMock(OrganisationService::class); - $organisations->method('getActiveOrganisation')->willReturn(null); - $organisations->method('createOrganisation')->willThrowException($failure); + private function service(?\Throwable $failure = null, ?OrganisationService $organisations = null): OrganisatieService { + if ($organisations === null) { + $organisations = $this->createMock(OrganisationService::class); + $organisations->method('getActiveOrganisation')->willReturn(null); + $organisations->method('createOrganisation')->willThrowException($failure); + } $container = $this->createMock(ContainerInterface::class); $container->method('get')->willReturn($organisations); diff --git a/tests/Unit/Service/OrganisationSlugTest.php b/tests/Unit/Service/OrganisationSlugTest.php new file mode 100644 index 000000000..79364bd77 --- /dev/null +++ b/tests/Unit/Service/OrganisationSlugTest.php @@ -0,0 +1,94 @@ + + * @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/specs/organisatie-service/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Stackiq\Tests\Unit\Service; + +use OCA\Stackiq\Service\OrganisationSlug; +use PHPUnit\Framework\TestCase; + +/** + * The slug matches OpenRegister's generateSlug(), and the name is the one OpenRegister receives. + * + * @covers \OCA\Stackiq\Service\OrganisationSlug + * + * @spec openspec/specs/organisatie-service/spec.md + */ +class OrganisationSlugTest extends TestCase { + + /** + * Names and the slugs OpenRegister gives them. + * + * @return array + */ + public static function slugs(): array { + return [ + 'plain' => ['Gemeente A', 'gemeente-a'], + 'runs of other characters' => [' Gemeente G & Zn! ', 'gemeente-g-zn'], + 'cut at 100' => [str_repeat('a', 120), str_repeat('a', 100)], + 'hyphen at position 100 is kept' => [str_repeat('a', 99) . ' b', str_repeat('a', 99) . '-'], + 'no letters or digits' => ['!!!', ''], + ]; + }//end slugs() + + /** + * The slug of a name. + * + * @param string $name The name. + * @param string $slug The slug OpenRegister gives it. + * + * @return void + * + * @dataProvider slugs + */ + public function testTheSlugOfAName(string $name, string $slug): void { + self::assertSame($slug, (new OrganisationSlug())->fromName(name: $name)); + }//end testTheSlugOfAName() + + /** + * Organisatie data and the name OpenRegister receives for it. + * + * @return array + */ + public static function names(): array { + $id = 'abcd1234-5678-4abc-8def-0123456789ab'; + return [ + 'its own name' => [['id' => $id, 'name' => 'Gemeente A'], 'Gemeente A'], + 'no name' => [['id' => $id], 'Organisation abcd1234'], + 'empty name' => [['id' => $id, 'name' => ''], 'Organisation abcd1234'], + 'named Unknown' => [['id' => $id, 'name' => 'Unknown'], 'Organisation abcd1234'], + 'named 0' => [['id' => $id, 'name' => '0'], 'Organisation abcd1234'], + 'named unknown' => [['id' => $id, 'name' => 'unknown'], 'unknown'], + 'no id' => [['name' => 'Unknown'], 'Organisation '], + ]; + }//end names() + + /** + * The name, and the slug of that name. + * + * @param array $objectData The organisatie object data. + * @param string $name The name OpenRegister receives. + * + * @return void + * + * @dataProvider names + */ + public function testTheNameOpenRegisterReceives(array $objectData, string $name): void { + $slugs = new OrganisationSlug(); + self::assertSame($name, $slugs->nameFor(objectData: $objectData)); + self::assertSame($slugs->fromName(name: $name), $slugs->fromObjectData(objectData: $objectData)); + }//end testTheNameOpenRegisterReceives() +}//end class diff --git a/tests/Unit/Service/OrganizationSyncContactPersonGoldenTest.php b/tests/Unit/Service/OrganizationSyncContactPersonGoldenTest.php index a3e40f167..6c74c4f51 100644 --- a/tests/Unit/Service/OrganizationSyncContactPersonGoldenTest.php +++ b/tests/Unit/Service/OrganizationSyncContactPersonGoldenTest.php @@ -305,6 +305,13 @@ function (string $uuid) { } ); + $mapper->method('findBySlug')->willReturnCallback( + function (string $slug) { + $this->trace[] = ['mapper.findBySlug', $slug]; + return ($this->plan['slugIndex'][$slug] ?? throw new DoesNotExistException('no slug')); + } + ); + $handler = $this->createMock(originalClassName: ContactPersonHandler::class); $handler->method('isFirstContactForOrganization')->willReturnCallback( function ($contact, $data) { @@ -604,6 +611,55 @@ public function testContactPerson(array $contactData, string $organizationUuid, }//end testContactPerson() + /** + * Event path cases: the doubles' answers, and whether a user account is made. + * + * @return array + */ + public static function eventCases(): array { + $missing = fn () => new DoesNotExistException('none'); + return [ + 'entity missing, no organisatie object' => [fn ($t) => ['org' => [$missing()], 'find' => [null]], false], + 'entity missing, backup lookup fails' => [fn ($t) => ['org' => [$missing()], 'find' => [new \RuntimeException('find failed')]], false], + 'entity missing, slug taken, none created' => [ + fn ($t) => [ + 'org' => [$missing(), $missing()], + 'find' => [$t->entity(['id' => 'org-e', 'name' => 'Gemeente A'], 'org-e'), null], + 'slugIndex' => ['gemeente-a' => self::organisation(active: true)], + ], + false, + ], + 'entity inactive' => [fn ($t) => ['org' => [self::organisation(active: false)]], false], + 'entity active' => [fn ($t) => ['org' => [self::organisation(active: true)]], true], + ]; + + }//end eventCases() + + /** + * The event path makes a user account only for an existing, active organisation + * entity, and an organisation without one ends the event normally. + * + * @param \Closure $plan The doubles' answers. + * @param bool $created Whether a user account is made. + * + * @dataProvider eventCases + * + * @return void + */ + public function testTheEventPathNeedsAnActiveOrganisationEntity(\Closure $plan, bool $created): void { + $this->plan = array_merge(['find' => [], 'save' => [], 'org' => []], $plan($this)); + $this->trace = []; + $stats = $this->service()->processSpecificContactPerson($this->entity(['organization' => 'org-e', 'email' => 'e@x.nl'], 'c-e')); + + self::assertSame(1, $stats['contactPersonsProcessed']); + self::assertSame([], $stats['errors']); + self::assertSame($created, in_array('handler.createUserAccount', array_column($this->trace, 0), true)); + $skipped = array_filter($this->trace, fn (array $call) => is_string($call[1] ?? null) === true && str_contains($call[1], 'Skipping user creation')); + self::assertSame($created === false, $skipped !== []); + self::assertNotContains('organisations.create', array_column($this->trace, 0)); + + }//end testTheEventPathNeedsAnActiveOrganisationEntity() + /** * The owner-update cases: object data, user, register, schema and override. * diff --git a/tests/Unit/Service/OrganizationSyncEntityGoldenTest.php b/tests/Unit/Service/OrganizationSyncEntityGoldenTest.php index c1f9336ff..199a3f6cd 100644 --- a/tests/Unit/Service/OrganizationSyncEntityGoldenTest.php +++ b/tests/Unit/Service/OrganizationSyncEntityGoldenTest.php @@ -375,6 +375,7 @@ function () { 'organisationMapper' => $mapper, 'organisationService' => $this->organisationService(), 'emailService' => $this->emailService(), + 'container' => $this->container(), ]; foreach ($properties as $name => $value) { $reflection->getProperty($name)->setValue($service, $value); @@ -384,6 +385,26 @@ function () { }//end service() + /** + * A container that answers OpenRegister's OrganisationService, whose default + * organisation is the plan's `default` (o-default when not set). + * + * @return \Psr\Container\ContainerInterface + */ + private function container(): \Psr\Container\ContainerInterface { + $organisations = $this->createMock(originalClassName: \OCA\OpenRegister\Service\OrganisationService::class); + $organisations->method('getDefaultOrganisationUuid')->willReturnCallback( + function () { + $this->trace[] = ['organisations.getDefaultOrganisationUuid']; + return array_key_exists('default', $this->plan) === true ? $this->plan['default'] : 'o-default'; + } + ); + $container = $this->createMock(originalClassName: \Psr\Container\ContainerInterface::class); + $container->method('get')->with('OCA\\OpenRegister\\Service\\OrganisationService')->willReturn($organisations); + return $container; + + }//end container() + /** * An organisation service double that records the organisations it creates. * @@ -579,7 +600,7 @@ public static function entityCases(): array { 'not found, created with mail' => [['name' => 'Gemeente H'], true, fn ($t) => ['org' => [$missing()], 'slug' => [$missing()]]], 'not found, created, mail not sent' => [['name' => 'Gemeente H2'], true, fn ($t) => ['org' => [$missing()], 'slug' => [$missing()], 'mail' => false]], 'not found, created without mail' => [['name' => 'Gemeente I'], false, fn ($t) => ['org' => [$missing()], 'slug' => [$missing()]]], - 'no name, created' => [['status' => 'actief'], true, fn ($t) => ['org' => [$missing()]]], + 'no name, created' => [['status' => 'actief'], true, fn ($t) => ['org' => [$missing()], 'slug' => [$missing()]]], 'creation fails' => [['name' => 'Gemeente J'], true, fn ($t) => ['org' => [$missing()], 'slug' => [$missing()], 'created' => null]], 'mapper fails' => [['name' => 'Gemeente K'], true, fn ($t) => ['org' => [new \RuntimeException('db down')]]], 'owner save fails' => [['name' => 'Gemeente L'], true, fn ($t) => ['org' => [$t->orgEntity('o-1', true)], 'save' => [new \RuntimeException('save failed')]]], @@ -644,6 +665,82 @@ public function testASameNamedRegistrationDoesNotCaptureTheExistingOrganisation( } }//end testASameNamedRegistrationDoesNotCaptureTheExistingOrganisation() + /** + * A registration with an empty, "Unknown" or "0" name reaches OpenRegister as + * "Organisation ". When another organisation + * already has that name's slug, nothing is created or taken over. + * + * @return void + */ + public function testAFallbackNamedRegistrationDoesNotCaptureTheExistingOrganisation(): void { + $id = 'abcd1234-5678-4abc-8def-0123456789ab'; + foreach (['no name' => [], 'empty' => ['name' => ''], 'Unknown' => ['name' => 'Unknown'], 'zero' => ['name' => '0']] as $case => $name) { + $this->trace = []; + $this->plan = [ + 'find' => [null], + 'save' => [], + 'org' => [new \OCP\AppFramework\Db\DoesNotExistException('missing')], + 'slugIndex' => ['organisation-abcd1234' => $this->orgEntity(uuid: 'o-other', active: true)], + ]; + $stats = ['entitiesCreated' => 0, 'entitiesUpdated' => 0]; + $object = array_merge(['id' => $id, 'status' => 'actief'], $name); + $entity = $this->service()->ensureOrganisationEntityPublic($this->entity(object: $object, uuid: 'o-1'), $stats, true); + + $writes = array_values(array_filter($this->trace, fn (array $call) => in_array($call[0], ['entity.setUuid', 'mapper.save', 'organisations.create', 'objects.saveObject', 'entity.setOwner', 'entity.setOrganisation'], true))); + self::assertNull($entity, $case); + self::assertSame([], $writes, $case); + self::assertSame(['o-1'], $stats['slugConflicts'] ?? null, $case); + } + }//end testAFallbackNamedRegistrationDoesNotCaptureTheExistingOrganisation() + + /** + * A fallback name whose slug is free is created, with the data as given. + * + * @return void + */ + public function testAFallbackNameWithAFreeSlugIsCreated(): void { + $this->trace = []; + $this->plan = [ + 'find' => [null], + 'save' => [], + 'org' => [new \OCP\AppFramework\Db\DoesNotExistException('missing')], + 'slugIndex' => ['unknown' => $this->orgEntity(uuid: 'o-other', active: true)], + ]; + $stats = ['entitiesCreated' => 0, 'entitiesUpdated' => 0]; + $object = ['id' => 'abcd1234-5678-4abc-8def-0123456789ab', 'name' => 'Unknown', 'status' => 'actief']; + $entity = $this->service()->ensureOrganisationEntityPublic($this->entity(object: $object, uuid: 'o-1'), $stats, false); + + self::assertNotNull($entity); + self::assertContains(['mapper.findBySlug', 'organisation-abcd1234'], $this->trace); + self::assertContains(['organisations.create', $object], $this->trace); + self::assertArrayNotHasKey('slugConflicts', $stats); + }//end testAFallbackNameWithAFreeSlugIsCreated() + + /** + * Checking a link answers what the link would, and writes nothing. + * + * @param string $outcome The expected outcome. + * @param \Closure $plan Builds the doubles' answers. + * @param array $writes The writes a link would make; a check makes none. + * + * @return void + * + * @dataProvider linkCases + */ + public function testCheckOrganisationLink(string $outcome, \Closure $plan, array $writes): void { + $this->trace = []; + $this->plan = array_merge(['find' => [], 'save' => [], 'org' => [], 'slug' => []], $plan($this)); + + $expected = $outcome; + if ($outcome === 'linked') { + $expected = null; + } + + self::assertSame($expected, $this->service()->checkOrganisationLink('o-1', 'o-old')); + $made = array_filter($this->trace, fn (array $call) => in_array($call[0], ['entity.setUuid', 'mapper.save', 'objects.saveObject'], true)); + self::assertSame([], array_values($made)); + }//end testCheckOrganisationLink() + /** * The link outcomes, and what a link writes. * @@ -658,6 +755,8 @@ public static function linkCases(): array { 'already linked' => ['already-linked', fn ($t) => ['find' => [$object($t, 'o-1')], 'org' => [$t->orgEntity('o-1', true)]], []], 'no organisation' => ['no-organisation', fn ($t) => ['find' => [$object($t, 'o-1')], 'org' => [$missing(), $missing()]], []], 'organisation in use' => ['organisation-in-use', fn ($t) => ['find' => [$object($t, 'o-1'), $object($t, 'o-old')], 'org' => [$missing(), $t->orgEntity('o-old', true)]], []], + 'default organisation' => ['default-organisation', fn ($t) => ['find' => [$object($t, 'o-1')], 'org' => [$missing(), $t->orgEntity('o-old', true)], 'default' => 'o-old'], []], + 'no default set' => ['linked', fn ($t) => ['find' => [$object($t, 'o-1'), null], 'org' => [$missing(), $t->orgEntity('o-old', true), $t->orgEntity('o-old', true)], 'default' => null], ['entity.setUuid', 'mapper.save', 'objects.saveObject']], ]; }//end linkCases() diff --git a/tests/Unit/Service/golden/organizationSyncContactPerson.json b/tests/Unit/Service/golden/organizationSyncContactPerson.json index 82000049b..4759eadbd 100644 --- a/tests/Unit/Service/golden/organizationSyncContactPerson.json +++ b/tests/Unit/Service/golden/organizationSyncContactPerson.json @@ -430,6 +430,17 @@ "register": "reg-v", "schema": "org-schema" } + ], + [ + "log.info", + "Skipping user creation - organization not active or not found in entity table", + { + "contactId": "c-4", + "organizationId": "org-4", + "organizationFound": false, + "organizationActive": false, + "email": "d@x.nl" + } ] ], "stats": { @@ -437,7 +448,7 @@ "usersCreated": 0, "errors": [] }, - "escaped": "Error: Call to a member function getActive() on null" + "escaped": null }, "contact: org entity missing, backup lookup fails": { "trace": [ @@ -495,6 +506,17 @@ "organizationId": "org-5", "error": "no config" } + ], + [ + "log.info", + "Skipping user creation - organization not active or not found in entity table", + { + "contactId": "c-5", + "organizationId": "org-5", + "organizationFound": false, + "organizationActive": false, + "email": "e@x.nl" + } ] ], "stats": { @@ -502,7 +524,7 @@ "usersCreated": 0, "errors": [] }, - "escaped": "Error: Call to a member function getActive() on null" + "escaped": null }, "contact: org entity missing, backup builds one": { "trace": [ @@ -595,6 +617,10 @@ "mapper.findByUuid", "org-6" ], + [ + "mapper.findBySlug", + "organisation" + ], [ "log.debug", "Creating new organisation entity", diff --git a/tests/Unit/Service/golden/organizationSyncEntity.json b/tests/Unit/Service/golden/organizationSyncEntity.json index c3900893b..568dbc4eb 100644 --- a/tests/Unit/Service/golden/organizationSyncEntity.json +++ b/tests/Unit/Service/golden/organizationSyncEntity.json @@ -1407,6 +1407,10 @@ "mapper.findByUuid", "o-1" ], + [ + "mapper.findBySlug", + "organisation" + ], [ "log.debug", "Creating new organisation entity", @@ -2135,6 +2139,10 @@ "mapper.findByUuid", "o-1" ], + [ + "mapper.findBySlug", + "organisation" + ], [ "log.debug", "Creating new organisation entity", From 37053c805913c3434b059347b7494904af3aa0ec Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Tue, 3 Nov 2026 23:29:27 +0100 Subject: [PATCH 3/3] refactor(command): write the link command's constructor on one line Its constructor matched ReferencesAuditCommand's line for line, which gate 122 counts as a new clone. Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/Command/OrganisationLinkCommand.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/Command/OrganisationLinkCommand.php b/lib/Command/OrganisationLinkCommand.php index 610df6a82..218cb8165 100644 --- a/lib/Command/OrganisationLinkCommand.php +++ b/lib/Command/OrganisationLinkCommand.php @@ -77,9 +77,7 @@ class OrganisationLinkCommand extends Command { * * @param ContainerInterface $container Container, for the lazy sync service resolve. */ - public function __construct( - private readonly ContainerInterface $container, - ) { + public function __construct(private readonly ContainerInterface $container) { parent::__construct(); }//end __construct()