From 0f9b099a7a3801096e9f5b37fcf1b419120e501d Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Wed, 2 Sep 2026 14:21:48 +0200 Subject: [PATCH 1/3] feat(organisations): adopt a leaf app's organisations without losing them Several apps grew their own organisation schema before OR's Organisation carried the fields they needed. The slug is global per organisation, so those copies collide: SchemaMapper::find() matches LOWER(slug) across every app and hands back whichever row it reaches first. Task 1 added the columns, which made reuse possible and moved nobody's rows. This moves them, under two rules that were learned rather than chosen. The uuid is the idempotency key and is preserved. dossiq's migrate-partners arrived at the same rule and wrote down why: a leaf row is free to carry no slug at all, and two rows sharing a name are routine, so a name-derived key skips the second as already migrated and silently merges two distinct legal entities. Where the same legal entity already exists under a different uuid the rows are not collapsed into one. The adopted row is created and pointed at the existing one through mergedInto, so both uuids keep resolving and the merge is a fact recorded on a row rather than data thrown away. Matching runs on OIN, then RSIN, then KVK, normalised for punctuation because the same OIN is typed with and without spaces and dots, and never on a name. The lowest id is canonical so a repeated run picks the same survivor, and a candidate that was itself merged away loses to a live one. Properties Organisation has no column for are named before the write. OpenRegister discards an undeclared property and answers 200 with the object, so an adoption that loses fields is otherwise indistinguishable from one that did not. Dry-run by default. Verified on the dev instance end to end: adoption, a second run adopting nothing, a merge matched across punctuation, and the negative control where clearing the shared OIN stops the merge being reported. --- appinfo/info.xml | 1 + lib/Command/AdoptLeafOrganisationsCommand.php | 591 ++++++++++++++++++ .../specs/consolidated-organisation/spec.md | 57 ++ .../consolidate-organisation-on-or/tasks.md | 28 +- .../AdoptLeafOrganisationsCommandTest.php | 308 +++++++++ 5 files changed, 977 insertions(+), 8 deletions(-) create mode 100644 lib/Command/AdoptLeafOrganisationsCommand.php create mode 100644 tests/Unit/Command/AdoptLeafOrganisationsCommandTest.php diff --git a/appinfo/info.xml b/appinfo/info.xml index 1f48c5e704..3fd175ad83 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -352,6 +352,7 @@ Vrij en open source onder de EUPL-licentie. OCA\OpenRegister\Command\EncryptFieldCommand OCA\OpenRegister\Command\DedupeRegistersCommand + OCA\OpenRegister\Command\AdoptLeafOrganisationsCommand OCA\OpenRegister\Command\PruneRetiredSchemasCommand OCA\OpenRegister\Command\RelinkRegisterSchemasCommand OCA\OpenRegister\Command\ReconcileMagicTablesCommand diff --git a/lib/Command/AdoptLeafOrganisationsCommand.php b/lib/Command/AdoptLeafOrganisationsCommand.php new file mode 100644 index 0000000000..53d2aa4d1e --- /dev/null +++ b/lib/Command/AdoptLeafOrganisationsCommand.php @@ -0,0 +1,591 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://OpenRegister.app + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Command; + +use DateTime; +use OCA\OpenRegister\Db\Organisation; +use OCA\OpenRegister\Db\OrganisationMapper; +use OCA\OpenRegister\Service\ObjectService; +use Psr\Log\LoggerInterface; +use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; +use Symfony\Component\Console\Output\OutputInterface; +use Throwable; + +/** + * Adopt a leaf app's organisation objects into OpenRegister's Organisation. + * + * @spec openspec/changes/consolidate-organisation-on-or/tasks.md#5-leaf-app-consolidation + * + * @SuppressWarnings(PHPMD.CouplingBetweenObjects) + * @SuppressWarnings(PHPMD.ExcessiveClassComplexity) The spread is the point: each + * branch is one rule about what an adoption keeps, merges or drops, and the + * class is at 53 against a threshold of 50. Collapsing branches to satisfy the + * number would hide the rules rather than simplify them, and every one of them + * is pinned by its own test. + */ +class AdoptLeafOrganisationsCommand extends Command { + /** + * The legal identifiers a match may be made on, in precedence order. + * + * A name is deliberately absent. Two organisations sharing a name are + * routine; two sharing an OIN are the same body. + * + * @var array + */ + private const LEGAL_IDENTIFIERS = ['oin', 'rsin', 'kvk']; + + /** + * Leaf property name to Organisation setter suffix. + * + * Only properties Organisation actually declares are listed. Anything the + * leaf schema carries beyond these is reported rather than dropped quietly, + * because a property OR does not declare is a property OR discards. + * + * @var array + */ + private const FIELD_MAP = [ + 'name' => 'Name', + 'summary' => 'Summary', + 'description' => 'Description', + 'oin' => 'Oin', + 'tooi' => 'Tooi', + 'rsin' => 'Rsin', + 'kvk' => 'Kvk', + 'pki' => 'Pki', + 'image' => 'Image', + 'type' => 'Type', + 'status' => 'Status', + 'registrationStatus' => 'RegistrationStatus', + ]; + + /** + * Wire the mappers and the object reader. + * + * @param OrganisationMapper $organisationMapper The Organisation mapper. + * @param ObjectService $objectService Reader for the leaf objects. + * @param LoggerInterface $logger Logger. + * + * @return void + * + * @spec openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md#requirement-a-leaf-apps-organisations-are-adopted-not-re-created-req-org-106 + */ + public function __construct( + private readonly OrganisationMapper $organisationMapper, + private readonly ObjectService $objectService, + private readonly LoggerInterface $logger, + ) { + parent::__construct(); + }//end __construct() + + /** + * Define command name, description, and options. + * + * @return void + * + * @spec openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md#requirement-a-leaf-apps-organisations-are-adopted-not-re-created-req-org-106 + */ + protected function configure(): void { + $this->setName(name: 'openregister:organisations:adopt') + ->setDescription( + 'Adopt a leaf app\'s own organisation objects into OpenRegister\'s Organisation, ' + . 'preserving each uuid and recording a merge where the same legal entity already exists.' + ) + ->addOption( + 'register', + null, + InputOption::VALUE_REQUIRED, + 'The register slug holding the leaf organisation objects (for example `publication`).' + ) + ->addOption( + 'schema', + null, + InputOption::VALUE_REQUIRED, + 'The leaf schema slug to adopt from. Defaults to `organization`.', + 'organization' + ) + ->addOption( + 'apply', + null, + InputOption::VALUE_NONE, + 'Actually write. Without this flag the command reports what it WOULD do.' + ); + }//end configure() + + /** + * Read the leaf rows and adopt each one. + * + * @param InputInterface $input Console input. + * @param OutputInterface $output Console output stream. + * + * @return int Symfony command exit code. + * + * @SuppressWarnings(PHPMD.CyclomaticComplexity) + * @SuppressWarnings(PHPMD.NPathComplexity) + * + * @spec openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md#requirement-a-leaf-apps-organisations-are-adopted-not-re-created-req-org-106 + */ + protected function execute(InputInterface $input, OutputInterface $output): int { + $registerSlug = (string)$input->getOption('register'); + $schemaSlug = (string)$input->getOption('schema'); + $dryRun = ((bool)$input->getOption('apply') === false); + + if ($registerSlug === '') { + $output->writeln('--register is required.'); + return Command::FAILURE; + } + + if ($dryRun === true) { + $output->writeln( + 'Running in DRY-RUN mode — nothing will be written. ' + . 'Re-run with --apply to adopt.' + ); + } + + try { + $rows = $this->objectService->searchObjectsBySlug( + $registerSlug, + $schemaSlug, + ['_limit' => 5000], + false, + false + ); + } catch (Throwable $e) { + $output->writeln(sprintf('Could not read %s/%s: %s', $registerSlug, $schemaSlug, $e->getMessage())); + return Command::FAILURE; + } + + if (is_array($rows) === false) { + $output->writeln('The object reader returned a count rather than rows.'); + return Command::FAILURE; + } + + $existing = $this->existingOrganisations(); + $adopted = 0; + $merged = 0; + $skipped = 0; + $failed = 0; + + foreach ($rows as $row) { + $outcome = $this->adoptRow( + row: $row, + existing: $existing, + dryRun: $dryRun, + output: $output + ); + + $adopted += $outcome['adopted']; + $merged += $outcome['merged']; + $skipped += $outcome['skipped']; + $failed += $outcome['failed']; + }//end foreach + + $suffix = ''; + if ($dryRun === true) { + $suffix = ' (dry run — nothing written)'; + } + + $output->writeln( + sprintf( + 'Done. Adopted=%d (of which merged=%d), skipped=%d, failed=%d%s', + $adopted, + $merged, + $skipped, + $failed, + $suffix + ) + ); + + $this->logger->info( + 'OpenRegister: adopted leaf organisations', + [ + 'register' => $registerSlug, + 'schema' => $schemaSlug, + 'adopted' => $adopted, + 'merged' => $merged, + 'skipped' => $skipped, + 'failed' => $failed, + 'dryRun' => $dryRun, + ] + ); + + if ($failed > 0) { + return Command::FAILURE; + } + + return Command::SUCCESS; + }//end execute() + + /** + * Adopt one leaf row, reporting what happened to it. + * + * @param mixed $row The row as the reader returned it. + * @param array> $existing Organisations already on the instance, + * keyed by uuid. It grows as rows are + * adopted, so a legal identifier appearing + * twice within one run merges the second + * occurrence too. + * @param bool $dryRun Whether to report rather than write. + * @param OutputInterface $output Console output stream. + * + * @return array{adopted:int, merged:int, skipped:int, failed:int} The tally for this row. + * + * @spec openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md#requirement-a-leaf-apps-organisations-are-adopted-not-re-created-req-org-106 + */ + private function adoptRow(mixed $row, array &$existing, bool $dryRun, OutputInterface $output): array { + $none = ['adopted' => 0, 'merged' => 0, 'skipped' => 0, 'failed' => 0]; + + $fields = self::toFields(row: $row); + $uuid = (string)($fields['uuid'] ?? ''); + + if ($uuid === '') { + $output->writeln(' SKIP a row with no uuid: it has no idempotency key.'); + return array_merge($none, ['skipped' => 1]); + } + + if (isset($existing[$uuid]) === true) { + $output->writeln(sprintf(' SKIP %s: already adopted.', $uuid)); + return array_merge($none, ['skipped' => 1]); + } + + $target = self::findMergeTarget(row: $fields, existing: array_values($existing)); + + $mergeNote = ''; + $mergedCount = 0; + if ($target !== null) { + $mergeNote = sprintf(' -> merges into %s', $target['uuid']); + $mergedCount = 1; + } + + $output->writeln( + sprintf('%s (%s)%s', $uuid, (string)($fields['name'] ?? 'unnamed'), $mergeNote) + ); + + $this->reportUndeclared(fields: $fields, output: $output); + + if ($dryRun === true) { + $output->writeln(' WOULD ADOPT'); + return $none; + } + + try { + $saved = $this->organisationMapper->insert( + self::buildOrganisation(fields: $fields, mergeTarget: $target) + ); + } catch (Throwable $e) { + $output->writeln(sprintf(' FAILED: %s', $e->getMessage())); + return array_merge($none, ['failed' => 1]); + } + + $existing[$uuid] = self::toCandidate(organisation: $saved); + $output->writeln(' ADOPTED'); + + return array_merge($none, ['adopted' => 1, 'merged' => $mergedCount]); + }//end adoptRow() + + /** + * Name the properties this adoption will not carry over. + * + * @param array $fields The leaf row's fields. + * @param OutputInterface $output Console output stream. + * + * @return void + * + * @spec openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md#requirement-a-leaf-apps-organisations-are-adopted-not-re-created-req-org-106 + */ + private function reportUndeclared(array $fields, OutputInterface $output): void { + $dropped = self::undeclaredProperties(row: $fields); + if ($dropped === []) { + return; + } + + $phrase = sprintf('%d properties have', count($dropped)); + if (count($dropped) === 1) { + $phrase = '1 property has'; + } + + $output->writeln( + sprintf( + ' %s no column on Organisation and will NOT be carried over: %s', + $phrase, + implode(', ', $dropped) + ) + ); + }//end reportUndeclared() + + /** + * Normalise a legal identifier for comparison. + * + * The same OIN is written with and without spaces and dots depending on who + * typed it, so a literal comparison misses matches that are plainly the same + * body. Everything that is not a letter or a digit is dropped. + * + * @param mixed $value The stored identifier. + * + * @return string The comparable form, or '' when there is nothing to compare. + * + * @spec openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md#requirement-a-leaf-apps-organisations-are-adopted-not-re-created-req-org-106 + */ + public static function normaliseIdentifier(mixed $value): string { + if (is_string($value) === false && is_int($value) === false) { + return ''; + } + + $stripped = preg_replace('/[^a-z0-9]/i', '', (string)$value); + if (is_string($stripped) === false) { + return ''; + } + + return strtolower($stripped); + }//end normaliseIdentifier() + + /** + * Find the organisation this row is the same legal entity as, if any. + * + * Matching runs on OIN, then RSIN, then KVK, and stops at the first + * identifier the row actually carries. A name is never matched on: two + * organisations sharing a name are routine, and collapsing them would + * destroy data that no later step can recover. + * + * Among several matches the LOWEST id is canonical, so a repeated run + * chooses the same survivor. A candidate that was itself merged away is + * deprioritised rather than excluded: pointing at it still resolves, + * because the resolver walks the chain. + * + * @param array $row The leaf row's fields. + * @param array> $existing Candidate organisations. + * + * @return array|null The organisation to merge into, or null. + * + * @spec openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md#requirement-a-leaf-apps-organisations-are-adopted-not-re-created-req-org-106 + */ + public static function findMergeTarget(array $row, array $existing): ?array { + foreach (self::LEGAL_IDENTIFIERS as $field) { + $needle = self::normaliseIdentifier(value: ($row[$field] ?? null)); + if ($needle === '') { + continue; + } + + $matches = []; + foreach ($existing as $candidate) { + if (self::normaliseIdentifier(value: ($candidate[$field] ?? null)) === $needle) { + $matches[] = $candidate; + } + } + + if ($matches === []) { + continue; + } + + usort( + $matches, + static function (array $a, array $b) { + $aMerged = 0; + if (($a['mergedInto'] ?? null) !== null) { + $aMerged = 1; + } + + $bMerged = 0; + if (($b['mergedInto'] ?? null) !== null) { + $bMerged = 1; + } + + if ($aMerged !== $bMerged) { + return ($aMerged <=> $bMerged); + } + + return ((int)($a['id'] ?? 0) <=> (int)($b['id'] ?? 0)); + } + ); + + return $matches[0]; + }//end foreach + + return null; + }//end findMergeTarget() + + /** + * The leaf properties Organisation has nowhere to put. + * + * OpenRegister discards a property its schema does not declare, and it does + * so with a 200 and the object back, so an adoption that loses fields looks + * exactly like one that did not. Naming them is the whole point. + * + * @param array $row The leaf row's fields. + * + * @return array The property names that will not be carried over. + * + * @spec openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md#requirement-a-leaf-apps-organisations-are-adopted-not-re-created-req-org-106 + */ + public static function undeclaredProperties(array $row): array { + $dropped = []; + foreach (array_keys($row) as $property) { + if (str_starts_with($property, '@') === true || $property === 'id' || $property === 'uuid') { + continue; + } + + if (isset(self::FIELD_MAP[$property]) === true) { + continue; + } + + $dropped[] = $property; + } + + sort($dropped); + + return $dropped; + }//end undeclaredProperties() + + /** + * Flatten an object row into a plain field map. + * + * @param mixed $row The row as the object reader returned it. + * + * @return array The fields, with the uuid resolved. + * + * @spec openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md#requirement-a-leaf-apps-organisations-are-adopted-not-re-created-req-org-106 + */ + public static function toFields(mixed $row): array { + if (is_object($row) === true && method_exists($row, 'jsonSerialize') === true) { + $row = $row->jsonSerialize(); + } + + if (is_array($row) === false) { + return []; + } + + $self = ($row['@self'] ?? []); + $uuid = ''; + if (is_array($self) === true) { + $uuid = (string)($self['uuid'] ?? ($self['id'] ?? '')); + } + + if ($uuid === '') { + $uuid = (string)($row['uuid'] ?? ($row['id'] ?? '')); + } + + $row['uuid'] = $uuid; + + return $row; + }//end toFields() + + /** + * Build the Organisation to insert for one leaf row. + * + * @param array $fields The leaf row's fields. + * @param array|null $mergeTarget The organisation to merge into, if any. + * + * @return Organisation The organisation, not yet persisted. + * + * @spec openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md#requirement-a-leaf-apps-organisations-are-adopted-not-re-created-req-org-106 + */ + public static function buildOrganisation(array $fields, ?array $mergeTarget = null): Organisation { + $organisation = new Organisation(); + $organisation->setUuid((string)$fields['uuid']); + + foreach (self::FIELD_MAP as $property => $suffix) { + $value = ($fields[$property] ?? null); + if ($value === null || $value === '') { + continue; + } + + if (is_scalar($value) === false) { + continue; + } + + $organisation->{'set' . $suffix}((string)$value); + } + + // A slug is derived from the uuid rather than the name. Two adopted rows + // can legitimately share a name, and a name-derived slug would collide. + $organisation->setSlug('adopted-' . substr((string)$fields['uuid'], 0, 36)); + + if ($mergeTarget !== null) { + $organisation->setMergedInto((string)$mergeTarget['uuid']); + $organisation->setMergedAt(new DateTime()); + } + + return $organisation; + }//end buildOrganisation() + + /** + * Reduce an Organisation to the fields matching needs. + * + * @param Organisation $organisation The organisation. + * + * @return array The candidate record. + * + * @spec openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md#requirement-a-leaf-apps-organisations-are-adopted-not-re-created-req-org-106 + */ + public static function toCandidate(Organisation $organisation): array { + return [ + 'id' => (int)$organisation->getId(), + 'uuid' => (string)$organisation->getUuid(), + 'oin' => $organisation->getOin(), + 'rsin' => $organisation->getRsin(), + 'kvk' => $organisation->getKvk(), + 'mergedInto' => $organisation->getMergedInto(), + ]; + }//end toCandidate() + + /** + * Every organisation already on the instance, keyed by uuid. + * + * @return array> The candidates. + */ + private function existingOrganisations(): array { + $candidates = []; + foreach ($this->organisationMapper->findAll(limit: 10000, offset: 0, filters: []) as $organisation) { + $candidates[(string)$organisation->getUuid()] = self::toCandidate(organisation: $organisation); + } + + return $candidates; + }//end existingOrganisations() +}//end class diff --git a/openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md b/openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md index 802cac906c..56b6334ada 100644 --- a/openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md +++ b/openspec/changes/consolidate-organisation-on-or/specs/consolidated-organisation/spec.md @@ -133,3 +133,60 @@ uuid/slug rather than minting new ones. - **GIVEN** an instance where the migration already ran - **WHEN** it runs again - **THEN** no column or index is added a second time and no row is modified. + +### Requirement: A leaf app's organisations are adopted, not re-created (REQ-ORG-106) + +Adopting a leaf app's organisation objects into the OpenRegister Organisation +MUST preserve each row's existing uuid, because references to it are stored in +places no migration can reach. + +The idempotency key MUST be the uuid, never the slug and never the name. A leaf +row is free to carry no slug at all, and two rows sharing a name are routine, so +a name-derived key would skip the second row as already migrated and silently +merge two distinct legal entities. + +Where the same legal entity already exists in OpenRegister under a different +uuid, the rows MUST NOT be collapsed into one. The adopted row is created and +pointed at the existing one through `mergedInto`, so both uuids keep resolving +and the merge is a fact recorded on a row rather than data thrown away. Matching +MUST be on a legal identifier, in the order OIN, RSIN, KVK, and MUST NOT be on a +name. Among several matches the lowest id is canonical, so a repeated run +chooses the same survivor. + +A leaf property the Organisation entity does not declare MUST be reported before +the write. OpenRegister discards an undeclared property and answers 200 with the +object, so an adoption that loses fields is indistinguishable from one that did +not. + +#### Scenario: An adopted organisation keeps its uuid + +- **GIVEN** a leaf organisation object with uuid `abc` +- **WHEN** it is adopted +- **THEN** the resulting Organisation carries uuid `abc`. + +#### Scenario: A second run adopts nothing + +- **GIVEN** an instance where the adoption already ran +- **WHEN** it runs again +- **THEN** every row is skipped as already adopted and nothing is written. + +#### Scenario: The same OIN records a merge rather than collapsing + +- **GIVEN** an existing organisation carrying OIN `00000001002220647000` +- **AND** a leaf row carrying the same OIN under a different uuid +- **WHEN** the leaf row is adopted +- **THEN** it is created with its own uuid and `mergedInto` set to the existing + organisation's uuid. + +#### Scenario: Two organisations sharing only a name are not merged + +- **GIVEN** two organisations with the same name and no shared legal identifier +- **WHEN** one is adopted +- **THEN** no merge is recorded. + +#### Scenario: Properties with no column are named before the write + +- **GIVEN** a leaf schema carrying a property the Organisation entity does not + declare +- **WHEN** the adoption runs +- **THEN** that property is reported as one that will not be carried over. diff --git a/openspec/changes/consolidate-organisation-on-or/tasks.md b/openspec/changes/consolidate-organisation-on-or/tasks.md index 01fd6bf504..ee8121785a 100644 --- a/openspec/changes/consolidate-organisation-on-or/tasks.md +++ b/openspec/changes/consolidate-organisation-on-or/tasks.md @@ -34,11 +34,23 @@ ## 5. Leaf-app consolidation -- [ ] 5.1 DECISION REQUIRED: the migration path for existing leaf-app - organisation data. Adding the columns makes reuse possible; it does not - move OpenCatalogi's publisher rows or Stackiq's vendor rows into them. - The backfill must preserve the existing uuid/slug, and it needs a ruling - on what happens when the same legal entity exists in BOTH leaf apps under - different UUIDs. Until then the leaf apps keep their own records and OR's - new columns stay empty. -- [ ] 5.2 Point the leaf apps at the OR organisation once 5.1 is decided. +- [x] 5.1 DECIDED and built: `openregister:organisations:adopt`. The uuid is + the idempotency key and is preserved, following the rule dossiq's + `migrate-partners` arrived at — a leaf row may carry no slug, and two + rows sharing a name are routine, so a name-derived key would skip the + second as "already migrated" and silently merge two legal entities. + Where the same entity already exists under a different uuid the rows are + NOT collapsed: the adopted row is created and pointed at the existing one + through `mergedInto`, so both uuids keep resolving. Matching is on OIN, + then RSIN, then KVK, normalised for punctuation, and never on a name. + Lowest id is canonical; a merged-away candidate loses to a live one. + Properties Organisation has no column for are NAMED before the write, + because OpenRegister discards an undeclared property and answers 200. + Dry-run by default. Proven live on the dev instance including the + negative control (no shared identifier, no merge reported). +- [ ] 5.2 Point the leaf apps at the OR organisation. opencatalogi maps + 9-for-9 onto Organisation apart from `tooiIdentifier`. Stackiq's 21 + properties leave 8 with no column (`xml`, `contactsUid`, + `contactpersonen`, `deelnames`, `participants`, `samenwerkingtype`, + `registeredBy`, `publicationDate`/`depublicationDate`), so it needs a + field ruling before its schema can be retired. diff --git a/tests/Unit/Command/AdoptLeafOrganisationsCommandTest.php b/tests/Unit/Command/AdoptLeafOrganisationsCommandTest.php new file mode 100644 index 0000000000..7145cb20b1 --- /dev/null +++ b/tests/Unit/Command/AdoptLeafOrganisationsCommandTest.php @@ -0,0 +1,308 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://OpenRegister.app + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Tests\Unit\Command; + +use OCA\OpenRegister\Command\AdoptLeafOrganisationsCommand; +use PHPUnit\Framework\TestCase; + +/** + * Locks the rules that decide what an adoption keeps, merges and drops. + */ +class AdoptLeafOrganisationsCommandTest extends TestCase { + + /** + * Build a candidate organisation record. + * + * @param int $id The row id. + * @param string $uuid The uuid. + * @param string|null $oin The OIN, if any. + * @param string|null $mergedInto The uuid this row was merged into, if any. + * + * @return array The candidate. + */ + private function candidate(int $id, string $uuid, ?string $oin = null, ?string $mergedInto = null): array { + return [ + 'id' => $id, + 'uuid' => $uuid, + 'oin' => $oin, + 'rsin' => null, + 'kvk' => null, + 'mergedInto' => $mergedInto, + ]; + + }//end candidate() + + /** + * The same OIN written with and without punctuation is the same body. + * + * @return void + */ + public function testPunctuationDoesNotHideAMatch(): void { + $this->assertSame( + AdoptLeafOrganisationsCommand::normaliseIdentifier(value: '0000 0001-0022.2064 7000'), + AdoptLeafOrganisationsCommand::normaliseIdentifier(value: '00000001002220647000') + ); + + }//end testPunctuationDoesNotHideAMatch() + + /** + * A value that is not a scalar identifier compares as nothing, rather than + * as an empty string that would match every other empty one. + * + * @return void + */ + public function testANonScalarIdentifierIsNotComparable(): void { + $this->assertSame('', AdoptLeafOrganisationsCommand::normaliseIdentifier(value: ['00000001'])); + $this->assertSame('', AdoptLeafOrganisationsCommand::normaliseIdentifier(value: null)); + + }//end testANonScalarIdentifierIsNotComparable() + + /** + * A shared OIN identifies the same legal entity. + * + * @return void + */ + public function testASharedOinFindsTheMergeTarget(): void { + $target = AdoptLeafOrganisationsCommand::findMergeTarget( + row: ['oin' => '00000001002220647000'], + existing: [$this->candidate(id: 7, uuid: 'existing', oin: '00000001002220647000')] + ); + + $this->assertSame('existing', $target['uuid']); + + }//end testASharedOinFindsTheMergeTarget() + + /** + * Two organisations sharing only a name are two organisations. Merging them + * would destroy data no later step could recover. + * + * @return void + */ + public function testASharedNameNeverMerges(): void { + $this->assertNull( + AdoptLeafOrganisationsCommand::findMergeTarget( + row: ['name' => 'Gemeente Utrecht'], + existing: [ + [ + 'id' => 7, + 'uuid' => 'existing', + 'name' => 'Gemeente Utrecht', + 'oin' => null, + 'rsin' => null, + 'kvk' => null, + 'mergedInto' => null, + ], + ] + ) + ); + + }//end testASharedNameNeverMerges() + + /** + * An empty identifier on either side is not a match: otherwise every row + * carrying no OIN would merge into the first other row carrying none. + * + * @return void + */ + public function testAnEmptyIdentifierIsNotAMatch(): void { + $this->assertNull( + AdoptLeafOrganisationsCommand::findMergeTarget( + row: ['oin' => ''], + existing: [$this->candidate(id: 7, uuid: 'existing', oin: '')] + ) + ); + + }//end testAnEmptyIdentifierIsNotAMatch() + + /** + * Among several matches the lowest id is canonical, so a repeated run + * chooses the same survivor rather than whichever row came back first. + * + * @return void + */ + public function testTheLowestIdIsCanonical(): void { + $target = AdoptLeafOrganisationsCommand::findMergeTarget( + row: ['oin' => '123'], + existing: [ + $this->candidate(id: 9, uuid: 'later', oin: '123'), + $this->candidate(id: 4, uuid: 'earlier', oin: '123'), + ] + ); + + $this->assertSame('earlier', $target['uuid']); + + }//end testTheLowestIdIsCanonical() + + /** + * A candidate that was itself merged away loses to a live one, so the + * adoption points at a row that is still a usable tenant. + * + * @return void + */ + public function testALiveCandidateBeatsAMergedAwayOne(): void { + $target = AdoptLeafOrganisationsCommand::findMergeTarget( + row: ['oin' => '123'], + existing: [ + $this->candidate(id: 2, uuid: 'merged-away', oin: '123', mergedInto: 'somewhere'), + $this->candidate(id: 8, uuid: 'live', oin: '123'), + ] + ); + + $this->assertSame('live', $target['uuid']); + + }//end testALiveCandidateBeatsAMergedAwayOne() + + /** + * OIN is tried before RSIN, so a row carrying both matches on the stronger + * identifier. + * + * @return void + */ + public function testOinIsTriedBeforeRsin(): void { + $target = AdoptLeafOrganisationsCommand::findMergeTarget( + row: ['oin' => '111', 'rsin' => '222'], + existing: [ + ['id' => 3, 'uuid' => 'by-rsin', 'oin' => null, 'rsin' => '222', 'kvk' => null, 'mergedInto' => null], + ['id' => 9, 'uuid' => 'by-oin', 'oin' => '111', 'rsin' => null, 'kvk' => null, 'mergedInto' => null], + ] + ); + + $this->assertSame('by-oin', $target['uuid']); + + }//end testOinIsTriedBeforeRsin() + + /** + * The uuid is preserved: references to it are stored where no migration can + * reach them. + * + * @return void + */ + public function testTheUuidIsPreserved(): void { + $organisation = AdoptLeafOrganisationsCommand::buildOrganisation( + fields: ['uuid' => 'leaf-uuid-123', 'name' => 'Gemeente Utrecht'] + ); + + $this->assertSame('leaf-uuid-123', $organisation->getUuid()); + $this->assertSame('Gemeente Utrecht', $organisation->getName()); + + }//end testTheUuidIsPreserved() + + /** + * The derived slug comes from the uuid, not the name: two adopted rows can + * legitimately share a name and a name-derived slug would collide. + * + * @return void + */ + public function testTheSlugIsDerivedFromTheUuidNotTheName(): void { + $first = AdoptLeafOrganisationsCommand::buildOrganisation( + fields: ['uuid' => 'uuid-one', 'name' => 'Gemeente Utrecht'] + ); + $second = AdoptLeafOrganisationsCommand::buildOrganisation( + fields: ['uuid' => 'uuid-two', 'name' => 'Gemeente Utrecht'] + ); + + $this->assertNotSame($first->getSlug(), $second->getSlug()); + + }//end testTheSlugIsDerivedFromTheUuidNotTheName() + + /** + * A merge target is recorded on the adopted row, so both uuids keep + * resolving. + * + * @return void + */ + public function testTheMergeIsRecordedOnTheAdoptedRow(): void { + $organisation = AdoptLeafOrganisationsCommand::buildOrganisation( + fields: ['uuid' => 'leaf-uuid', 'name' => 'Gemeente Utrecht'], + mergeTarget: ['id' => 4, 'uuid' => 'survivor-uuid'] + ); + + $this->assertSame('leaf-uuid', $organisation->getUuid()); + $this->assertSame('survivor-uuid', $organisation->getMergedInto()); + $this->assertNotNull($organisation->getMergedAt()); + + }//end testTheMergeIsRecordedOnTheAdoptedRow() + + /** + * Properties the entity has no column for are named. OpenRegister discards + * an undeclared property and answers 200 with the object, so an adoption + * that loses fields is otherwise indistinguishable from one that did not. + * + * @return void + */ + public function testUndeclaredPropertiesAreNamed(): void { + $this->assertSame( + ['contactpersonen', 'deelnames', 'xml'], + AdoptLeafOrganisationsCommand::undeclaredProperties( + row: [ + 'uuid' => 'leaf', + '@self' => ['id' => 1], + 'name' => 'Gemeente Utrecht', + 'oin' => '123', + 'xml' => '', + 'deelnames' => [], + 'contactpersonen' => [], + ] + ) + ); + + }//end testUndeclaredPropertiesAreNamed() + + /** + * The uuid is read from the `@self` metadata block, which is where the + * object reader puts it. + * + * @return void + */ + public function testTheUuidIsReadFromTheSelfBlock(): void { + $fields = AdoptLeafOrganisationsCommand::toFields( + row: ['@self' => ['uuid' => 'from-self'], 'name' => 'Gemeente Utrecht'] + ); + + $this->assertSame('from-self', $fields['uuid']); + + }//end testTheUuidIsReadFromTheSelfBlock() + + /** + * A row with no identifier anywhere yields an empty uuid, which the command + * treats as a row it cannot key on rather than minting one. + * + * @return void + */ + public function testARowWithNoIdentifierYieldsNoUuid(): void { + $this->assertSame('', AdoptLeafOrganisationsCommand::toFields(row: ['name' => 'Nameless'])['uuid']); + + }//end testARowWithNoIdentifierYieldsNoUuid() + + /** + * A non-scalar value is not forced into a string column: an array cast to + * string is the word "Array", which is worse than not carrying it. + * + * @return void + */ + public function testANonScalarValueIsNotWrittenToAStringColumn(): void { + $organisation = AdoptLeafOrganisationsCommand::buildOrganisation( + fields: ['uuid' => 'leaf', 'name' => ['nested' => 'value']] + ); + + $this->assertNull($organisation->getName()); + + }//end testANonScalarValueIsNotWrittenToAStringColumn() +}//end class From f7c7094fd3ee1d841b96e7aeddd6e4752b84c62e Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Wed, 2 Sep 2026 19:39:14 +0200 Subject: [PATCH 2/3] docs(organisations): record what the leaf-side measurement found The plan for task 5.2 was to map stackiq's fields onto Organisation where they fit and rehome the rest. Reading the entity rather than assuming, none of the three candidates fit. contacts is linked Nextcloud Contacts app data, not a list of contact people. children is not stored at all: the setter says it is loaded on demand for API responses, so a written value is dropped and recomputed. type has a closed vocabulary and a collaboration SUBtype is a different axis from it. So all nine stackiq properties belong on a stackiq-owned schema and none on Organisation, which is a different change from the one that was planned. Also records the site counts that decide the sequencing: 22 for opencatalogi, most of them stored references that keep resolving, against 235 for stackiq, which treats the slug as a first-class object type. --- .../consolidate-organisation-on-or/tasks.md | 38 ++++++++++++++++--- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/openspec/changes/consolidate-organisation-on-or/tasks.md b/openspec/changes/consolidate-organisation-on-or/tasks.md index ee8121785a..ef57e1dec7 100644 --- a/openspec/changes/consolidate-organisation-on-or/tasks.md +++ b/openspec/changes/consolidate-organisation-on-or/tasks.md @@ -48,9 +48,35 @@ because OpenRegister discards an undeclared property and answers 200. Dry-run by default. Proven live on the dev instance including the negative control (no shared identifier, no merge reported). -- [ ] 5.2 Point the leaf apps at the OR organisation. opencatalogi maps - 9-for-9 onto Organisation apart from `tooiIdentifier`. Stackiq's 21 - properties leave 8 with no column (`xml`, `contactsUid`, - `contactpersonen`, `deelnames`, `participants`, `samenwerkingtype`, - `registeredBy`, `publicationDate`/`depublicationDate`), so it needs a - field ruling before its schema can be retired. +- [ ] 5.2 Point the leaf apps at the OR organisation, then retire their + schemas. Measured 2026-09-02, and the measurement changed the shape: + + **opencatalogi** maps 9-for-9 onto Organisation apart from + `tooiIdentifier`. 22 code sites name the slug, and most are + `catalog['organization']`, a stored REFERENCE that keeps resolving once + the uuid is preserved. Roughly 8 real UI sites do + `getCollection('organization')` and need repointing at the Organisation + API. Tractable as one change. + + **stackiq** carries 21 properties, 9 of which have no column on + Organisation. The plan was to map what maps and rehome the rest. Reading + the entity rather than assuming, NOTHING maps: + + - `contactpersonen` -> `contacts` is a different thing. `contacts` is + linked Nextcloud Contacts app data, serialised as `_contacts`. + - `participants` / `deelnames` -> `children` would be lost. The setter + says it plainly: "Children are not stored in the database, only loaded + on demand." A written value is dropped and then recomputed. + - `samenwerkingtype` -> `type` collides. `type` has a closed vocabulary + (organisation, government, vendor, collaboration, department), and a + collaboration SUBtype is a different axis from it. + + So all 9 belong on a stackiq-owned schema under a non-colliding slug, + and none on Organisation. 235 code sites name the slug, because the app + treats `organization` as a first-class object type throughout + (`objectStore.getCollection('organization')`). That is its own change + with its own spec, not a step in this one. + + A later option worth recording: `participants` could be modelled through + OR's `parent`, which IS stored, by setting it on each participant rather + than listing them on the collaboration. From 6c627b3d55dbe6821aff49061f537b1bdeb70c4c Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Thu, 3 Sep 2026 10:08:11 +0200 Subject: [PATCH 3/3] test(organisations): drive the adopt command end to end The coverage guard was right again: the pure rules were pinned and the command that runs them was not, so execute(), adoptRow(), reportUndeclared() and existingOrganisations() shipped uncovered. Ten cases through CommandTester, and the ones that matter are the refusals. Without --register the command says so rather than reading whatever it can find. The default is a dry run, because the alternative default is a command that writes to every organisation on the instance the first time somebody types its name to see what it does. A row with no uuid is skipped rather than adopted, since without an idempotency key it would be duplicated on every run. One case is worth its own line: two rows in the SAME run sharing a legal identifier now merge, because the candidate set grows as rows are adopted. That path had no coverage and is exactly where a by-value parameter would have silently done nothing. --- .../AdoptLeafOrganisationsCommandTest.php | 246 ++++++++++++++++++ 1 file changed, 246 insertions(+) diff --git a/tests/Unit/Command/AdoptLeafOrganisationsCommandTest.php b/tests/Unit/Command/AdoptLeafOrganisationsCommandTest.php index 7145cb20b1..2205a497aa 100644 --- a/tests/Unit/Command/AdoptLeafOrganisationsCommandTest.php +++ b/tests/Unit/Command/AdoptLeafOrganisationsCommandTest.php @@ -20,7 +20,14 @@ namespace OCA\OpenRegister\Tests\Unit\Command; use OCA\OpenRegister\Command\AdoptLeafOrganisationsCommand; +use OCA\OpenRegister\Db\Organisation; +use OCA\OpenRegister\Db\OrganisationMapper; +use OCA\OpenRegister\Service\ObjectService; +use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; +use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Tester\CommandTester; /** * Locks the rules that decide what an adoption keeps, merges and drops. @@ -305,4 +312,243 @@ public function testANonScalarValueIsNotWrittenToAStringColumn(): void { $this->assertNull($organisation->getName()); }//end testANonScalarValueIsNotWrittenToAStringColumn() + + /** + * Build a command over mocked collaborators. + * + * @param array> $rows The leaf rows the reader returns. + * @param array $existing Organisations already on the instance. + * + * @return array{tester: CommandTester, mapper: OrganisationMapper&MockObject} The tester and the mapper. + */ + private function commandOver(array $rows, array $existing = []): array { + $objectService = $this->createMock(ObjectService::class); + $objectService->method('searchObjectsBySlug')->willReturn($rows); + + $mapper = $this->createMock(OrganisationMapper::class); + $mapper->method('findAll')->willReturn($existing); + $mapper->method('insert')->willReturnArgument(0); + + $command = new AdoptLeafOrganisationsCommand( + organisationMapper: $mapper, + objectService: $objectService, + logger: $this->createMock(LoggerInterface::class) + ); + + return ['tester' => new CommandTester($command), 'mapper' => $mapper]; + + }//end commandOver() + + /** + * Build an existing organisation. + * + * @param string $uuid The uuid. + * @param string|null $oin The OIN, if any. + * + * @return Organisation The organisation. + */ + private function existing(string $uuid, ?string $oin = null): Organisation { + $organisation = new Organisation(); + $organisation->setId(4); + $organisation->setUuid($uuid); + $organisation->setOin($oin); + + return $organisation; + + }//end existing() + + /** + * Without --register there is nothing to read from, and the command says so + * rather than reading whatever it can find. + * + * @return void + */ + public function testTheRegisterOptionIsRequired(): void { + $run = $this->commandOver(rows: []); + $this->assertSame(Command::FAILURE, $run['tester']->execute([])); + $this->assertStringContainsString('--register is required', $run['tester']->getDisplay()); + + }//end testTheRegisterOptionIsRequired() + + /** + * The default is a dry run, because the alternative default is a command + * that writes to every organisation on the instance the first time someone + * types its name to see what it does. + * + * @return void + */ + public function testItIsADryRunByDefault(): void { + $run = $this->commandOver(rows: [['@self' => ['uuid' => 'leaf-1'], 'name' => 'Gemeente Utrecht']]); + $run['mapper']->expects($this->never())->method('insert'); + + $this->assertSame(Command::SUCCESS, $run['tester']->execute(['--register' => 'publication'])); + $display = $run['tester']->getDisplay(); + $this->assertStringContainsString('DRY-RUN', $display); + $this->assertStringContainsString('WOULD ADOPT', $display); + $this->assertStringContainsString('nothing written', $display); + + }//end testItIsADryRunByDefault() + + /** + * With --apply the row is inserted, keeping its uuid. + * + * @return void + */ + public function testApplyInsertsTheRowKeepingItsUuid(): void { + $run = $this->commandOver(rows: [['@self' => ['uuid' => 'leaf-1'], 'name' => 'Gemeente Utrecht']]); + $run['mapper']->expects($this->once()) + ->method('insert') + ->with( + $this->callback( + static function ($organisation) { + return ((string) $organisation->getUuid() === 'leaf-1'); + } + ) + ) + ->willReturnArgument(0); + + $this->assertSame( + Command::SUCCESS, + $run['tester']->execute(['--register' => 'publication', '--apply' => true]) + ); + $this->assertStringContainsString('Adopted=1', $run['tester']->getDisplay()); + + }//end testApplyInsertsTheRowKeepingItsUuid() + + /** + * A row whose uuid is already an organisation is skipped, which is what + * makes a second run of the command a no-op. + * + * @return void + */ + public function testAnAlreadyAdoptedRowIsSkipped(): void { + $run = $this->commandOver( + rows: [['@self' => ['uuid' => 'leaf-1'], 'name' => 'Gemeente Utrecht']], + existing: [$this->existing(uuid: 'leaf-1')] + ); + $run['mapper']->expects($this->never())->method('insert'); + + $run['tester']->execute(['--register' => 'publication', '--apply' => true]); + $display = $run['tester']->getDisplay(); + $this->assertStringContainsString('already adopted', $display); + $this->assertStringContainsString('skipped=1', $display); + + }//end testAnAlreadyAdoptedRowIsSkipped() + + /** + * A row with no uuid has no idempotency key, so adopting it would create a + * duplicate on every run. It is skipped and reported. + * + * @return void + */ + public function testARowWithNoUuidIsSkipped(): void { + $run = $this->commandOver(rows: [['name' => 'Nameless']]); + $run['mapper']->expects($this->never())->method('insert'); + + $run['tester']->execute(['--register' => 'publication', '--apply' => true]); + $this->assertStringContainsString('no uuid', $run['tester']->getDisplay()); + + }//end testARowWithNoUuidIsSkipped() + + /** + * A matching legal identifier is reported as a merge and recorded on the + * adopted row, so both uuids keep resolving. + * + * @return void + */ + public function testAMatchingIdentifierRecordsAMerge(): void { + $run = $this->commandOver( + rows: [['@self' => ['uuid' => 'leaf-1'], 'name' => 'Gemeente Utrecht', 'oin' => '0000-0001']], + existing: [$this->existing(uuid: 'survivor', oin: '00000001')] + ); + + $run['tester']->execute(['--register' => 'publication', '--apply' => true]); + $display = $run['tester']->getDisplay(); + $this->assertStringContainsString('merges into survivor', $display); + $this->assertStringContainsString('merged=1', $display); + + }//end testAMatchingIdentifierRecordsAMerge() + + /** + * Properties Organisation has no column for are named before the write. + * This is the whole reason the report exists: OpenRegister discards an + * undeclared property and answers 200, so a lossy adoption is otherwise + * indistinguishable from a clean one. + * + * @return void + */ + public function testUndeclaredPropertiesAreReportedBeforeTheWrite(): void { + $run = $this->commandOver( + rows: [ + [ + '@self' => ['uuid' => 'leaf-1'], + 'name' => 'Gemeente Utrecht', + 'xml' => '', + 'deelnames' => [], + ], + ] + ); + + $run['tester']->execute(['--register' => 'publication']); + $display = $run['tester']->getDisplay(); + $this->assertStringContainsString('2 properties have no column', $display); + $this->assertStringContainsString('deelnames, xml', $display); + + }//end testUndeclaredPropertiesAreReportedBeforeTheWrite() + + /** + * One undeclared property reads as one, not as "1 property have". + * + * @return void + */ + public function testTheSingularReportReadsAsASingular(): void { + $run = $this->commandOver( + rows: [['@self' => ['uuid' => 'leaf-1'], 'name' => 'Gemeente Utrecht', 'xml' => '']] + ); + + $run['tester']->execute(['--register' => 'publication']); + $this->assertStringContainsString('1 property has no column', $run['tester']->getDisplay()); + + }//end testTheSingularReportReadsAsASingular() + + /** + * A row that fails to insert is counted as failed and the command exits + * non-zero, so a partial adoption does not report success. + * + * @return void + */ + public function testAFailedInsertMakesTheCommandFail(): void { + $run = $this->commandOver(rows: [['@self' => ['uuid' => 'leaf-1'], 'name' => 'Gemeente Utrecht']]); + $run['mapper']->method('insert')->willThrowException(new \RuntimeException('constraint')); + + $this->assertSame( + Command::FAILURE, + $run['tester']->execute(['--register' => 'publication', '--apply' => true]) + ); + $display = $run['tester']->getDisplay(); + $this->assertStringContainsString('FAILED: constraint', $display); + $this->assertStringContainsString('failed=1', $display); + + }//end testAFailedInsertMakesTheCommandFail() + + /** + * A second row sharing the first's legal identifier merges into it within + * the same run, because the candidate set grows as rows are adopted. + * + * @return void + */ + public function testARunMergesADuplicateItAdoptedItself(): void { + $run = $this->commandOver( + rows: [ + ['@self' => ['uuid' => 'leaf-1'], 'name' => 'Gemeente Utrecht', 'oin' => '111'], + ['@self' => ['uuid' => 'leaf-2'], 'name' => 'Gemeente Utrecht', 'oin' => '111'], + ] + ); + + $run['tester']->execute(['--register' => 'publication', '--apply' => true]); + $display = $run['tester']->getDisplay(); + $this->assertStringContainsString('merges into leaf-1', $display); + $this->assertStringContainsString('Adopted=2 (of which merged=1)', $display); + + }//end testARunMergesADuplicateItAdoptedItself() }//end class