From c215ea42fe571977e62122d2737a80457bb7dc52 Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Fri, 2 Oct 2026 15:10:58 +0200 Subject: [PATCH 01/10] fix(import): a failed CMDB or ArchiMate import is reported as failed, not running or completed A CMDB import that throws outside a row (a progress write, an \Error) now marks its operation failed before the error propagates, and the controller answers IMPORT_FAILED for any Throwable instead of a bare 500. An ArchiMate import that fails is stored with status `failed` at the percentage it reached rather than `completed` at 100%. Both use a new ProgressTracker::failOperation(). A cancel of a CMDB import that is no longer running answers OPERATION_NOT_FOUND and leaves no cancel flag in the cache. Refs: WOO-589 Co-Authored-By: Claude Opus 5.5 (1M context) --- lib/Controller/CmdbImportController.php | 2 +- lib/Service/ArchiMateImportService.php | 3 +- lib/Service/CmdbExportImportService.php | 48 +++++++++------- lib/Service/ProgressTracker.php | 36 ++++++++++++ .../Controller/CmdbImportControllerTest.php | 15 +++++ .../Service/ArchiMateImportProgressTest.php | 20 +++++++ .../Service/CmdbExportImportServiceTest.php | 56 +++++++++++++++++++ .../Service/ProgressTrackerCancelTest.php | 23 ++++++++ 8 files changed, 181 insertions(+), 22 deletions(-) diff --git a/lib/Controller/CmdbImportController.php b/lib/Controller/CmdbImportController.php index 821deaec..a8425feb 100644 --- a/lib/Controller/CmdbImportController.php +++ b/lib/Controller/CmdbImportController.php @@ -101,7 +101,7 @@ public function import(): JSONResponse { ['error' => $e->getErrorCode(), 'details' => $e->getDetails(), 'reason' => $e->getMessage()] ); return $this->fromException(e: $e); - } catch (\Exception $e) { + } catch (\Throwable $e) { $this->logger->error('CmdbImportController: import failed', ['exception' => $e]); return $this->error(code: 'IMPORT_FAILED', status: Http::STATUS_INTERNAL_SERVER_ERROR); } diff --git a/lib/Service/ArchiMateImportService.php b/lib/Service/ArchiMateImportService.php index f3cc5e21..7f06a382 100644 --- a/lib/Service/ArchiMateImportService.php +++ b/lib/Service/ArchiMateImportService.php @@ -498,8 +498,7 @@ public function importArchiMateFileFromPathOptimized(array $options = []): array ); if ($this->operationId !== null) { - $this->progressTracker->addError(message: $e->getMessage()); - $this->progressTracker->completeOperation(); + $this->progressTracker->failOperation(message: $e->getMessage()); } return [ diff --git a/lib/Service/CmdbExportImportService.php b/lib/Service/CmdbExportImportService.php index 793e9b83..93e988a6 100644 --- a/lib/Service/CmdbExportImportService.php +++ b/lib/Service/CmdbExportImportService.php @@ -217,7 +217,7 @@ public function supportsMissingRecords(string $mode): bool { * * @param string $operationId The operation id. * - * @return bool False when no `cmdb_import` operation has this id. + * @return bool False when no running `cmdb_import` operation has this id. * * @spec openspec/changes/cmdb-export-import/tasks.md#task-7 */ @@ -227,7 +227,10 @@ public function requestCancel(string $operationId): bool { } $progress = $this->progressTracker->getProgress(operationId: $operationId); - if (is_array($progress) === false || ($progress['operation_type'] ?? null) !== self::OPERATION_TYPE) { + if (is_array($progress) === false + || ($progress['operation_type'] ?? null) !== self::OPERATION_TYPE + || ($progress['status'] ?? null) !== 'running' + ) { return false; } @@ -282,25 +285,32 @@ public function import(string $path, array $options): array { $this->progressTracker->setPhase(phase: 'processing_elements', data: ['total_items' => count($rows)]); $updateExisting = (($options['updateExisting'] ?? true) !== false); - foreach ($rows as $index => $row) { - if ($this->progressTracker->isCancelRequested(operationId: $operationId) === true) { - $report->markCancelled(); - break; - } + try { + foreach ($rows as $index => $row) { + if ($this->progressTracker->isCancelRequested(operationId: $operationId) === true) { + $report->markCancelled(); + break; + } - $this->processRow( - row: $row, - municipalityUuid: $municipality['uuid'], - updateExisting: $updateExisting, - startedAt: $startedAt, - date1904: $workbook['date1904'], - report: $report - ); - $this->progressTracker->updateProgress(processedItems: ($index + 1)); - } + $this->processRow( + row: $row, + municipalityUuid: $municipality['uuid'], + updateExisting: $updateExisting, + startedAt: $startedAt, + date1904: $workbook['date1904'], + report: $report + ); + $this->progressTracker->updateProgress(processedItems: ($index + 1)); + } - $result = $report->toArray(); - $this->finishOperation(report: $result); + $result = $report->toArray(); + $this->finishOperation(report: $result); + } catch (Throwable $e) { + // Rows catch their own errors; this is the run itself failing, so the + // operation stops as failed instead of staying running until it expires. + $this->progressTracker->failOperation(message: $e->getMessage()); + throw $e; + }//end try $this->logger->info( 'CmdbExportImportService: import finished', diff --git a/lib/Service/ProgressTracker.php b/lib/Service/ProgressTracker.php index 8bc62e4e..7a1dacbb 100644 --- a/lib/Service/ProgressTracker.php +++ b/lib/Service/ProgressTracker.php @@ -361,6 +361,42 @@ public function completeOperation(array $finalStatistics = []): void { ); }//end completeOperation() + /** + * Mark the current operation as failed, keeping the percentage it reached. + * + * A page following the operation then sees it stop as failed rather than + * as running until the snapshot expires, or as completed at 100%. + * + * @param string $message Why the operation failed + * + * @return void + * + * @spec openspec/specs/progress-tracking/spec.md + */ + public function failOperation(string $message): void { + $this->progress['errors'][] = [ + 'message' => $message, + 'context' => [], + 'timestamp' => time(), + ]; + $this->progress['phase_description'] = 'Failed'; + $this->progress['status'] = 'failed'; + $this->progress['estimated_completion'] = time(); + $this->saveProgress(); + + if ($this->progress['operation_id'] !== null) { + $this->store->remove(key: 'cancel_' . $this->progress['operation_id']); + } + + $this->logger->error( + 'Operation failed', + [ + 'operation_id' => $this->progress['operation_id'], + 'message' => $message, + ] + ); + }//end failOperation() + /** * Ask a running operation to stop. * diff --git a/tests/Unit/Controller/CmdbImportControllerTest.php b/tests/Unit/Controller/CmdbImportControllerTest.php index 665a2317..d190d803 100644 --- a/tests/Unit/Controller/CmdbImportControllerTest.php +++ b/tests/Unit/Controller/CmdbImportControllerTest.php @@ -289,6 +289,21 @@ public function testAnUnexpectedErrorIsAGeneric500(): void { $this->assertStringNotContainsString('SQLSTATE', $response->getData()['message']); }//end testAnUnexpectedErrorIsAGeneric500() + /** + * A PHP Error (not an Exception) from the import is the same generic 500, not a bare one. + * + * @return void + */ + public function testAnUnexpectedPhpErrorIsAGeneric500(): void { + $service = $this->service(); + $service->method('import')->willThrowException(new \TypeError('internal detail')); + + $response = $this->controller(file: $this->file(path: $this->upload()), params: ['municipalityName' => 'Gemeente Voorbeeldstad'], service: $service)->import(); + + $this->assertSame(500, $response->getStatus()); + $this->assertSame('IMPORT_FAILED', $response->getData()['error']); + }//end testAnUnexpectedPhpErrorIsAGeneric500() + /** * A valid upload passes the options through and answers 200 with the report. * diff --git a/tests/Unit/Service/ArchiMateImportProgressTest.php b/tests/Unit/Service/ArchiMateImportProgressTest.php index a15dcbf8..5257ddb6 100644 --- a/tests/Unit/Service/ArchiMateImportProgressTest.php +++ b/tests/Unit/Service/ArchiMateImportProgressTest.php @@ -165,6 +165,26 @@ function (array $objects): array { $this->assertSame('cancelled', $this->tracker()->getProgress('archimate_import_abc12345')['status']); }//end testACancelDuringTheFirstGroupStopsBeforeTheSecond() + /** + * An import that throws is stored as failed at the percentage it reached, not completed at 100%. + * + * @return void + */ + public function testAFailedImportIsStoredAsFailed(): void { + $service = $this->importService($this->tracker()); + (new \ReflectionProperty(ArchiMateImportService::class, 'cachedConfig'))->setValue($service, ['userId' => 'admin']); + + $result = $service->importArchiMateFileFromPathOptimized( + ['operationId' => 'archimate_import_abc12345', 'filePath' => '/does/not/exist.xml'] + ); + + $this->assertFalse($result['success']); + $stored = $this->tracker()->getProgress('archimate_import_abc12345'); + $this->assertSame('failed', $stored['status']); + $this->assertLessThan(100, $stored['percentage']); + $this->assertStringContainsString('File not found', $stored['errors'][0]['message']); + }//end testAFailedImportIsStoredAsFailed() + /** * An id that does not match the pattern is ignored: no operation, no cancel checks. * diff --git a/tests/Unit/Service/CmdbExportImportServiceTest.php b/tests/Unit/Service/CmdbExportImportServiceTest.php index cde666c2..b3a914f3 100644 --- a/tests/Unit/Service/CmdbExportImportServiceTest.php +++ b/tests/Unit/Service/CmdbExportImportServiceTest.php @@ -105,6 +105,13 @@ class CmdbExportImportServiceTest extends TestCase { */ private array $cache = []; + /** + * Thrown by the next write to the cache, once. + * + * @var \Throwable|null + */ + private ?\Throwable $cacheFailure = null; + /** * Every log line, message plus encoded context. * @@ -132,6 +139,7 @@ protected function setUp(): void { $this->contacts = []; $this->contactsEnabled = true; $this->cache = []; + $this->cacheFailure = null; $this->logLines = []; }//end setUp() @@ -298,6 +306,12 @@ private function progressTracker(): ProgressTracker { $cache->method('get')->willReturnCallback(fn ($key) => ($this->cache[$key] ?? null)); $cache->method('set')->willReturnCallback( function ($key, $value): bool { + if ($this->cacheFailure !== null) { + $failure = $this->cacheFailure; + $this->cacheFailure = null; + throw $failure; + } + $this->cache[$key] = $value; return true; } @@ -1160,6 +1174,48 @@ public function testCancelNeedsACmdbOperation(): void { $this->assertFalse($service->requestCancel(operationId: 'cmdb-not-mine-1')); }//end testCancelNeedsACmdbOperation() + /** + * Cancel answers false for an import that already finished, and leaves no cancel flag behind. + * + * @return void + */ + public function testCancelNeedsARunningImport(): void { + $this->seedOrganisation(uuid: 'muni-1', name: 'Gemeente Voorbeeldstad', type: 'Municipality'); + $service = $this->service(reader: $this->rowsReader(rows: [$this->row(appId: '1', row: 2)])); + $service->import(path: '', options: ['municipalityUuid' => 'muni-1', 'operationId' => 'cmdb-finished-1']); + + $this->assertFalse($service->requestCancel(operationId: 'cmdb-finished-1')); + $this->assertArrayNotHasKey('cancel_cmdb-finished-1', $this->cache); + }//end testCancelNeedsARunningImport() + + /** + * A failure outside a row stops the operation as failed instead of leaving it running. + * + * @return void + */ + public function testAFailureOutsideARowMarksTheOperationFailed(): void { + $this->seedOrganisation(uuid: 'muni-1', name: 'Gemeente Voorbeeldstad', type: 'Municipality'); + $rows = [$this->row(appId: '1', row: 2), $this->row(appId: '2', row: 3)]; + $service = $this->service(reader: $this->rowsReader(rows: $rows)); + $this->beforeSave = function (int $schema): void { + if ($schema === self::USAGE) { + // The progress write after this row fails, outside every row boundary. + $this->cacheFailure = new \Error('cache went away'); + } + }; + + try { + $service->import(path: '', options: ['municipalityUuid' => 'muni-1', 'operationId' => 'cmdb-failing-1']); + $this->fail('the import should have thrown'); + } catch (\Error $e) { + $this->assertSame('cache went away', $e->getMessage()); + } + + $stored = $this->cache['progress_cmdb-failing-1']; + $this->assertSame('failed', $stored['status']); + $this->assertSame('cache went away', $stored['errors'][0]['message']); + }//end testAFailureOutsideARowMarksTheOperationFailed() + /** * Without a mapping engine, or without configuration, nothing is read or written. * diff --git a/tests/Unit/Service/ProgressTrackerCancelTest.php b/tests/Unit/Service/ProgressTrackerCancelTest.php index e1e7ba27..2ab3cc61 100644 --- a/tests/Unit/Service/ProgressTrackerCancelTest.php +++ b/tests/Unit/Service/ProgressTrackerCancelTest.php @@ -126,4 +126,27 @@ public function testACancelledOperationIsStoredAsCancelled(): void { $this->assertSame('cancelled', $stored['status']); $this->assertSame(4, $stored['processed_items']); }//end testACancelledOperationIsStoredAsCancelled() + + /** + * A failed operation is stored as failed with its reason and the counts it reached, and its cancel flag is cleared. + * + * @return void + */ + public function testAFailedOperationIsStoredAsFailed(): void { + $importRequest = $this->tracker(); + $importRequest->startOperation(operationType: 'archimate_import', operationId: 'archimate_import_abc12345'); + $importRequest->setPhase('processing_elements', ['total_items' => 10]); + $importRequest->updateProgress(processedItems: 4); + $percentage = $importRequest->getProgress()['percentage']; + $this->tracker()->setCancelRequested('archimate_import_abc12345'); + + $importRequest->failOperation('Database went away'); + + $stored = $this->tracker()->getProgress('archimate_import_abc12345'); + $this->assertSame('failed', $stored['status']); + $this->assertSame(4, $stored['processed_items']); + $this->assertSame($percentage, $stored['percentage']); + $this->assertSame('Database went away', $stored['errors'][0]['message']); + $this->assertFalse($importRequest->isCancelRequested('archimate_import_abc12345')); + }//end testAFailedOperationIsStoredAsFailed() }//end class From 795a3729239a227fafa8bdaac15b3ea2c6ca3022 Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Fri, 2 Oct 2026 15:18:44 +0200 Subject: [PATCH 02/10] fix(itsm): a service desk set-up that cannot be checked is refused, and stores the organisation's uuid A missing flow template or a preflight that throws no longer escapes as a bare 500: setUp() logs it and answers the `{created: false, message}` refusal the settings page renders, and integriq's connection report hears about it. The organisation the admin picks is looked up by id, uuid or slug; the set-up now writes the found object's uuid into the flows and the stored config, since the flows compare it with a usage's consumer uuid. Refs: WOO-589 Co-Authored-By: Claude Opus 5.5 (1M context) --- lib/Service/ItsmExchangeService.php | 40 ++++++++++++------- .../Unit/Service/ItsmExchangeServiceTest.php | 40 ++++++++++++++++++- 2 files changed, 65 insertions(+), 15 deletions(-) diff --git a/lib/Service/ItsmExchangeService.php b/lib/Service/ItsmExchangeService.php index bd903231..e14991f3 100644 --- a/lib/Service/ItsmExchangeService.php +++ b/lib/Service/ItsmExchangeService.php @@ -265,20 +265,29 @@ public function buildFlows(string $desk, string $organisation, string $runAs, st * @spec openspec/changes/sharing-itsm-exchange/specs/itsm-exchange/spec.md#requirement-req-itx-001-an-administrator-sets-up-the-exchange-without-stackiq-holding-a-credential */ public function setUp(string $desk, string $organisation, string $runAs, string $templateId = ''): array { - $source = $this->preconditions(desk: $desk, organisation: $organisation); - if (is_string($source) === true) { - return $this->refuse(message: $source); + $found = $this->preconditions(desk: $desk, organisation: $organisation); + if (is_string($found) === true) { + return $this->refuse(message: $found); } - $flows = $this->buildFlows( - desk: $desk, - organisation: $organisation, - runAs: $runAs, - location: (string) ($source['location'] ?? ''), - templateId: $templateId - ); + // The flows compare this with a usage's consumer uuid, so store the uuid + // even when the admin gave an id or a slug. + $organisation = $found['organisation']; + try { + $flows = $this->buildFlows( + desk: $desk, + organisation: $organisation, + runAs: $runAs, + location: (string) ($found['source']['location'] ?? ''), + templateId: $templateId + ); + + $blocking = $this->blockingFindings(flows: $flows); + } catch (Throwable $e) { + $this->logger->error('[ItsmExchangeService] Checking the exchange flows failed', ['exception' => $e]); + return $this->refuse(message: 'Nothing was created. The flows could not be checked: ' . $e->getMessage()); + } - $blocking = $this->blockingFindings(flows: $flows); if ($blocking !== []) { $first = (array) reset($blocking); $entry = (array) ($first[0] ?? []); @@ -317,7 +326,9 @@ public function setUp(string $desk, string $organisation, string $runAs, string * @param string $desk The desk key. * @param string $organisation The organisation uuid. * - * @return array|string The integriq source, or why the set-up cannot start. + * @return array{source: array, organisation: string}|string The integriq source and the + * organisation's uuid, or why the + * set-up cannot start. */ private function preconditions(string $desk, string $organisation): array|string { if ($this->gateway->available() === false) { @@ -328,7 +339,8 @@ private function preconditions(string $desk, string $organisation): array|string return 'Unknown service desk "' . $desk . '".'; } - if ($this->gateway->findObject(register: self::REGISTER, schema: 'organization', id: $organisation) === null) { + $found = $this->gateway->findObject(register: self::REGISTER, schema: 'organization', id: $organisation); + if ($found === null || (string) ($found['uuid'] ?? '') === '') { return 'The organisation ' . $organisation . ' does not exist in stackiq.'; } @@ -338,7 +350,7 @@ private function preconditions(string $desk, string $organisation): array|string return 'Integriq has no source "' . $profile['source'] . '". Add the ' . $profile['label'] . ' source in integriq first.'; } - return $source; + return ['source' => $source, 'organisation' => (string) $found['uuid']]; }//end preconditions() /** diff --git a/tests/Unit/Service/ItsmExchangeServiceTest.php b/tests/Unit/Service/ItsmExchangeServiceTest.php index 44f31755..84b49772 100644 --- a/tests/Unit/Service/ItsmExchangeServiceTest.php +++ b/tests/Unit/Service/ItsmExchangeServiceTest.php @@ -103,7 +103,7 @@ protected function setUp(): void { $this->gateway->method('available')->willReturn(true); $this->gateway->method('findObject')->willReturnCallback( static function (string $register, string $schema, string $id): ?array { - if ($register === 'stackiq' && $schema === 'organization' && $id === self::ORG) { + if ($register === 'stackiq' && $schema === 'organization' && in_array($id, [self::ORG, 'gemeente-rotterdam'], true) === true) { return ['uuid' => self::ORG, 'name' => 'Gemeente Rotterdam']; } @@ -214,4 +214,42 @@ public function testWhatIsMissingIsNamed(): void { $this->assertStringContainsString('does not exist', $service->setUp(desk: 'topdesk', organisation: 'nope', runAs: 'admin')['message']); $this->assertStringContainsString('no source "servicenow"', $service->setUp(desk: 'servicenow', organisation: self::ORG, runAs: 'admin')['message']); }//end testWhatIsMissingIsNamed() + + /** + * An organisation given by slug is stored, and written into the flows, as its uuid. + * + * @return void + */ + public function testAnOrganisationGivenBySlugIsStoredAsItsUuid(): void { + $this->gateway->method('inspect')->willReturn(['blocking' => [], 'warnings' => []]); + $saved = []; + $this->gateway->method('saveAndPublish')->willReturnCallback( + static function (array $flow) use (&$saved): string { + $saved[] = $flow; + return 'flow-' . count($saved); + } + ); + + $this->service()->setUp(desk: 'topdesk', organisation: 'gemeente-rotterdam', runAs: 'admin'); + + $this->assertSame(self::ORG, json_decode((string) $this->settings['itsm_exchange'], true)['organisation']); + $this->assertStringContainsString(self::ORG, (string) json_encode($saved[0])); + $this->assertStringNotContainsString('gemeente-rotterdam', (string) json_encode($saved)); + }//end testAnOrganisationGivenBySlugIsStoredAsItsUuid() + + /** + * A preflight that throws is a refusal the page can show, reported to integriq, not an error. + * + * @return void + */ + public function testAPreflightThatThrowsIsARefusal(): void { + $this->gateway->method('inspect')->willThrowException(new \RuntimeException('preflight unresolvable')); + $this->gateway->expects($this->never())->method('saveAndPublish'); + $this->reports->expects($this->once())->method('itsmSetUp')->with(false, $this->stringContains('preflight unresolvable')); + + $result = $this->service()->setUp(desk: 'topdesk', organisation: self::ORG, runAs: 'admin'); + + $this->assertFalse($result['created']); + $this->assertStringContainsString('Nothing was created', $result['message']); + }//end testAPreflightThatThrowsIsARefusal() }//end class From 825ac329e1ea0c094bce67540d455d4b9b36301d Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Fri, 2 Oct 2026 15:18:44 +0200 Subject: [PATCH 03/10] fix(itsm): the file import caps its size, reads only xlsx data and stops at the row limit An upload over 10 MiB is refused before it is read. An .xlsx file is always read with PhpSpreadsheet's Xlsx reader in data-only mode, and a formula gives the value Excel cached instead of being calculated. CSV and XLSX rows are read one by one and reading stops one row past MAX_ROWS. A file flow the engine refuses to run answers `{started: false, message}` instead of a bare 500. Refs: WOO-589 Co-Authored-By: Claude Opus 5.5 (1M context) --- lib/Service/ItsmFileImportService.php | 143 ++++++++++++++---- .../Service/ItsmFileImportServiceTest.php | 99 +++++++++++- 2 files changed, 210 insertions(+), 32 deletions(-) diff --git a/lib/Service/ItsmFileImportService.php b/lib/Service/ItsmFileImportService.php index 6efad1ce..5af1b79e 100644 --- a/lib/Service/ItsmFileImportService.php +++ b/lib/Service/ItsmFileImportService.php @@ -29,6 +29,7 @@ use OCA\Stackiq\AppInfo\Application; use OCA\Stackiq\Service\Itsm\ItsmFlowGateway; use OCP\IAppConfig; +use Psr\Log\LoggerInterface; use RuntimeException; use Throwable; @@ -46,6 +47,20 @@ class ItsmFileImportService { */ public const MAX_ROWS = 5000; + /** + * The largest file one import reads, in bytes (10 MiB, as the CMDB import). + * + * @var integer + */ + public const MAX_FILE_BYTES = 10485760; + + /** + * PhpSpreadsheet's XLSX reader, as OpenRegister ships it. + * + * @var string + */ + public const XLSX_READER = '\PhpOffice\PhpSpreadsheet\Reader\Xlsx'; + /** * The column every row must fill. * @@ -58,10 +73,12 @@ class ItsmFileImportService { * * @param ItsmFlowGateway $gateway OpenRegister's flow store. * @param IAppConfig $appConfig The app settings. + * @param LoggerInterface $logger Logs a flow that could not be started. */ public function __construct( private readonly ItsmFlowGateway $gateway, private readonly IAppConfig $appConfig, + private readonly LoggerInterface $logger, ) { }//end __construct() @@ -85,6 +102,11 @@ public function import(string $path, string $name): array { return ['started' => false, 'message' => 'Set up the exchange first. The file import uses the flow the set-up creates.']; } + $size = @filesize($path); + if ($size === false || $size > self::MAX_FILE_BYTES) { + return ['started' => false, 'message' => 'The file is larger than ' . (self::MAX_FILE_BYTES / 1048576) . ' MB; one import takes at most that.']; + } + try { $rows = $this->readRows(path: $path, name: $name); } catch (Throwable $e) { @@ -96,7 +118,12 @@ public function import(string $path, string $name): array { return ['started' => false, 'message' => $problem]; } - $run = $this->gateway->run(uuid: $flow, payload: ['rows' => $rows]); + try { + $run = $this->gateway->run(uuid: $flow, payload: ['rows' => $rows]); + } catch (Throwable $e) { + $this->logger->error('[ItsmFileImportService] Starting the file import flow failed', ['exception' => $e]); + return ['started' => false, 'message' => 'The file import flow could not be started: ' . $e->getMessage()]; + } return ['started' => true, 'run' => $run, 'rows' => count($rows)]; }//end import() @@ -107,6 +134,9 @@ public function import(string $path, string $name): array { * @param string $path The file. * @param string $name Its name. * + * Reading stops one row past MAX_ROWS, so an oversized file is refused + * without reading the rest of it. + * * @return list> The rows, empty cells left out. * * @throws RuntimeException When the type is not CSV or XLSX, or XLSX cannot be read here. @@ -114,11 +144,18 @@ public function import(string $path, string $name): array { * @spec openspec/changes/sharing-itsm-exchange/specs/itsm-exchange/spec.md#requirement-req-itx-006-a-file-feeds-the-same-import */ public function readRows(string $path, string $name): array { - $table = $this->readTable(path: $path, name: $name); - - $header = array_map(static fn ($cell): string => trim((string) $cell), (array) array_shift($table)); + $header = null; $rows = []; - foreach ($table as $cells) { + foreach ($this->readTable(path: $path, name: $name) as $cells) { + if ($header === null) { + $header = array_map(static fn ($cell): string => trim((string) $cell), $cells); + continue; + } + + if (count($rows) > self::MAX_ROWS) { + break; + } + $row = []; foreach ($header as $index => $column) { $cell = trim((string) ($cells[$index] ?? '')); @@ -141,11 +178,11 @@ public function readRows(string $path, string $name): array { * @param string $path The file. * @param string $name Its name. * - * @return list> The cells. + * @return iterable> The cells, row by row. * * @throws RuntimeException When the type is not CSV or XLSX. */ - private function readTable(string $path, string $name): array { + private function readTable(string $path, string $name): iterable { $extension = strtolower(pathinfo($name, PATHINFO_EXTENSION)); if ($extension === 'csv') { return $this->readCsv(path: $path); @@ -173,7 +210,7 @@ public function checkRows(array $rows): ?string { } if (count($rows) > self::MAX_ROWS) { - return 'The file holds ' . count($rows) . ' rows; one import takes at most ' . self::MAX_ROWS . '.'; + return 'The file holds more than ' . self::MAX_ROWS . ' rows; one import takes at most ' . self::MAX_ROWS . '.'; } foreach ($rows as $index => $row) { @@ -190,49 +227,93 @@ public function checkRows(array $rows): ?string { * * @param string $path The file. * - * @return list> The cells. + * @return \Generator> The cells, row by row; the file closes when reading stops. */ - private function readCsv(string $path): array { + private function readCsv(string $path): \Generator { $handle = fopen($path, 'r'); if ($handle === false) { throw new RuntimeException('cannot open the uploaded file'); } - $first = (string) fgets($handle); - $delimiter = ','; - if (substr_count($first, ';') > substr_count($first, ',')) { - $delimiter = ';'; - } + try { + $first = (string) fgets($handle); + $delimiter = ','; + if (substr_count($first, ';') > substr_count($first, ',')) { + $delimiter = ';'; + } - rewind($handle); - $table = []; - while (($cells = fgetcsv($handle, null, $delimiter, '"', '\\')) !== false) { - $table[] = array_map(static fn ($cell): string => (string) $cell, $cells); - } + rewind($handle); + $isFirst = true; + while (($cells = fgetcsv($handle, null, $delimiter, '"', '\\')) !== false) { + $cells = array_map(static fn ($cell): string => (string) $cell, $cells); + if ($isFirst === true && isset($cells[0]) === true) { + $cells[0] = (string) preg_replace('/^\xEF\xBB\xBF/', '', $cells[0]); + } - fclose($handle); - if (isset($table[0][0]) === true) { - $table[0][0] = preg_replace('/^\xEF\xBB\xBF/', '', $table[0][0]); + $isFirst = false; + yield $cells; + } + } finally { + fclose($handle); } - - return $table; }//end readCsv() /** * Read the first sheet of an XLSX file into rows of cells, with PhpSpreadsheet as OpenRegister ships it. * + * Always the XLSX reader, whatever the content looks like, with data only. + * A formula gives the value Excel cached; it is never calculated here. + * * @param string $path The file. * - * @return list> The cells. + * @return \Generator> The cells, row by row; the workbook is released when reading stops. */ - private function readXlsx(string $path): array { - $factory = '\PhpOffice\PhpSpreadsheet\IOFactory'; - if (class_exists($factory) === false) { + private function readXlsx(string $path): \Generator { + $readerClass = self::XLSX_READER; + if (class_exists($readerClass) === false) { throw new RuntimeException('reading .xlsx needs PhpSpreadsheet, which OpenRegister provides; save the sheet as .csv instead'); } - $sheet = $factory::load($path)->getActiveSheet()->toArray(null, true, false, false); + $reader = new $readerClass(); + $reader->setReadDataOnly(true); + $spreadsheet = $reader->load($path); + try { + foreach ($spreadsheet->getActiveSheet()->getRowIterator() as $row) { + $iterator = $row->getCellIterator(); + $iterator->setIterateOnlyExistingCells(false); + $cells = []; + foreach ($iterator as $cell) { + $cells[] = self::cellText(cell: $cell); + } - return array_map(static fn ($cells): array => array_map(static fn ($cell): string => (string) $cell, (array) $cells), (array) $sheet); + yield $cells; + } + } finally { + $spreadsheet->disconnectWorksheets(); + } }//end readXlsx() + + /** + * The text of one XLSX cell; for a formula, the value Excel cached. + * + * @param object $cell The PhpSpreadsheet cell. + * + * @return string The text, or an empty string for a value that is not text or a number. + */ + private static function cellText(object $cell): string { + $value = $cell->getValue(); + if ($cell->getDataType() === 'f') { + $value = $cell->getOldCalculatedValue(); + } + + if (is_object($value) === true && method_exists($value, 'getPlainText') === true) { + return (string) $value->getPlainText(); + } + + if (is_scalar($value) === false) { + return ''; + } + + return (string) $value; + }//end cellText() }//end class diff --git a/tests/Unit/Service/ItsmFileImportServiceTest.php b/tests/Unit/Service/ItsmFileImportServiceTest.php index 1817134e..62290cd0 100644 --- a/tests/Unit/Service/ItsmFileImportServiceTest.php +++ b/tests/Unit/Service/ItsmFileImportServiceTest.php @@ -18,11 +18,16 @@ namespace OCA\Stackiq\Tests\Unit\Service; +require_once __DIR__ . '/../Support/CmdbTestSupport.php'; + use OCA\Stackiq\Service\Itsm\ItsmFlowGateway; use OCA\Stackiq\Service\ItsmFileImportService; +use OCA\Stackiq\Tests\Unit\Support\CmdbTestSupport; use OCP\IAppConfig; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; +use RuntimeException; /** * Asserts the reading, the refusals and the run. @@ -82,7 +87,7 @@ private function service(?string $flow): ItsmFileImportService { $config->method('getValueString')->willReturn($setting); - return new ItsmFileImportService(gateway: $this->gateway, appConfig: $config); + return new ItsmFileImportService(gateway: $this->gateway, appConfig: $config, logger: $this->createMock(LoggerInterface::class)); }//end service() /** @@ -99,6 +104,28 @@ private function csv(string $content): string { return $path; }//end csv() + /** + * Write a one-sheet XLSX file by hand, so its formula cells carry exactly the cached value given. + * + * @param string $sheetRows The `row` elements of the sheet. + * + * @return string The path. + */ + private function xlsx(string $sheetRows): string { + $path = (string) tempnam(sys_get_temp_dir(), 'itsm'); + $this->files[] = $path; + $zip = new \ZipArchive(); + $zip->open($path, \ZipArchive::OVERWRITE); + $zip->addFromString('[Content_Types].xml', ''); + $zip->addFromString('_rels/.rels', ''); + $zip->addFromString('xl/workbook.xml', ''); + $zip->addFromString('xl/_rels/workbook.xml.rels', ''); + $zip->addFromString('xl/worksheets/sheet1.xml', '' . $sheetRows . ''); + $zip->close(); + + return $path; + }//end xlsx() + /** * A semicolon CSV with a byte order mark reads as rows keyed by header, empty cells left out. * @@ -164,4 +191,74 @@ public function testNoSetUpAndAnotherTypeAreRefused(): void { $this->assertStringContainsString('Set up the exchange first', $this->service(flow: null)->import(path: $path, name: 'a.csv')['message']); $this->assertStringContainsString('only .csv and .xlsx', $this->service(flow: 'file-flow')->import(path: $path, name: 'a.ods')['message']); }//end testNoSetUpAndAnotherTypeAreRefused() + + /** + * A file over the byte limit is refused before it is read. + * + * @return void + */ + public function testAFileOverTheSizeLimitIsRefused(): void { + $this->gateway->expects($this->never())->method('run'); + $path = $this->csv("recordId\n" . str_repeat('A', ItsmFileImportService::MAX_FILE_BYTES) . "\n"); + + $result = $this->service(flow: 'file-flow')->import(path: $path, name: 'big.csv'); + + $this->assertFalse($result['started']); + $this->assertStringContainsString('larger than 10 MB', $result['message']); + }//end testAFileOverTheSizeLimitIsRefused() + + /** + * Reading stops one row past the row cap, and the file is refused. + * + * @return void + */ + public function testReadingStopsOneRowPastTheCap(): void { + $this->gateway->expects($this->never())->method('run'); + $lines = ['recordId']; + for ($i = 1; $i <= (ItsmFileImportService::MAX_ROWS + 50); $i++) { + $lines[] = 'A-' . $i; + } + + $path = $this->csv(implode("\n", $lines) . "\n"); + $service = $this->service(flow: 'file-flow'); + + $this->assertCount(ItsmFileImportService::MAX_ROWS + 1, $service->readRows(path: $path, name: 'many.csv')); + $this->assertStringContainsString('more than ' . ItsmFileImportService::MAX_ROWS . ' rows', $service->import(path: $path, name: 'many.csv')['message']); + }//end testReadingStopsOneRowPastTheCap() + + /** + * A flow the engine refuses to run is the refusal envelope, not an error. + * + * @return void + */ + public function testAFlowThatCannotRunIsRefused(): void { + $path = $this->csv("recordId\nA-1\n"); + $this->gateway->method('run')->willThrowException(new RuntimeException('flow is not runnable')); + + $result = $this->service(flow: 'file-flow')->import(path: $path, name: 'a.csv'); + + $this->assertFalse($result['started']); + $this->assertStringContainsString('flow is not runnable', $result['message']); + }//end testAFlowThatCannotRunIsRefused() + + /** + * An XLSX file gives the value a formula cached, not a recalculated one, and a CSV named .xlsx is not read as CSV. + * + * @return void + */ + public function testAnXlsxGivesCachedFormulaValuesAndOnlyXlsxIsRead(): void { + if (CmdbTestSupport::loadPhpSpreadsheet() === false) { + $this->markTestSkipped('PhpSpreadsheet comes from an OpenRegister vendor directory, which is not available.'); + } + + $path = $this->xlsx( + 'recordIdname' + . 'A-1CONCATENATE("Zaak","systeem")Cached name' + ); + + $this->assertSame([['recordId' => 'A-1', 'name' => 'Cached name']], $this->service(flow: null)->readRows(path: $path, name: 'landscape.xlsx')); + + $csv = $this->csv("recordId\nA-1\n"); + $this->assertStringContainsString('could not be read', $this->service(flow: 'file-flow')->import(path: $csv, name: 'landscape.xlsx')['message']); + }//end testAnXlsxGivesCachedFormulaValuesAndOnlyXlsxIsRead() }//end class From 3297dee058db5205311c57e3748777e07bbcf45c Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Fri, 2 Oct 2026 15:19:53 +0200 Subject: [PATCH 04/10] fix(publication): a module's TOPdesk reference fields read for signed-in users only The CMDB import adds externalId, externalNumber, externalKey, externalCreatedAt and externalModifiedAt to `module`, and externalKey embeds the importing municipality's uuid. They now carry the same `authenticated` read rule as the other service desk references, so an anonymous reader of a published module no longer sees them. The module schema goes to 0.3.7 so installed registers pick the rules up. Refs: WOO-589 Co-Authored-By: Claude Opus 5.5 (1M context) --- .../register.d/publication-field-rules.json | 37 ++++++++++++++++++- .../Settings/PublicationFieldRulesTest.php | 12 +++++- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/lib/Settings/register.d/publication-field-rules.json b/lib/Settings/register.d/publication-field-rules.json index ffe8456c..a939858a 100644 --- a/lib/Settings/register.d/publication-field-rules.json +++ b/lib/Settings/register.d/publication-field-rules.json @@ -3,7 +3,7 @@ "components": { "schemas": { "module": { - "version": "0.3.6", + "version": "0.3.7", "properties": { "contactPerson": { "authorization": { @@ -32,6 +32,41 @@ "authenticated" ] } + }, + "externalId": { + "authorization": { + "read": [ + "authenticated" + ] + } + }, + "externalNumber": { + "authorization": { + "read": [ + "authenticated" + ] + } + }, + "externalKey": { + "authorization": { + "read": [ + "authenticated" + ] + } + }, + "externalCreatedAt": { + "authorization": { + "read": [ + "authenticated" + ] + } + }, + "externalModifiedAt": { + "authorization": { + "read": [ + "authenticated" + ] + } } } }, diff --git a/tests/Unit/Settings/PublicationFieldRulesTest.php b/tests/Unit/Settings/PublicationFieldRulesTest.php index 26bbafd5..c47505f3 100644 --- a/tests/Unit/Settings/PublicationFieldRulesTest.php +++ b/tests/Unit/Settings/PublicationFieldRulesTest.php @@ -75,7 +75,17 @@ private function register(): array { public function testPrivateFieldsReadForSignedInUsersOnly(): void { $schemas = $this->register()['components']['schemas']; $private = [ - 'module' => ['contactPerson', 'usages', 'dpiaDocumentRef', 'verwerkingsregisterRef'], + 'module' => [ + 'contactPerson', + 'usages', + 'dpiaDocumentRef', + 'verwerkingsregisterRef', + 'externalId', + 'externalNumber', + 'externalKey', + 'externalCreatedAt', + 'externalModifiedAt', + ], 'moduleVersion' => ['usages'], 'catalogService' => ['contactPerson'], 'connection' => ['provider', 'serviceDeskRecordId', 'serviceDeskUrl', 'longDescription'], From 06b3835029e7e6d4bb0772f4ac113eaca8ca086c Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Fri, 2 Oct 2026 15:32:32 +0200 Subject: [PATCH 05/10] fix(publication): a depublished or deleted application takes all its versions out of public view The mirror that copies an application's publication onto its versions failed open in several ways. It now: - reads a module's versions page by page instead of stopping at 500; - saves the mirrored fields without validation, so a version holding older data the schema no longer accepts still follows its module; - clears the versions of a deleted module (ObjectDeletedEvent); - logs at critical level when a write that would take a version out of public view fails, since that version stays readable anonymously. A module update that leaves publicationDate and registeredBy as they were no longer searches its versions, which keeps ordinary module saves from paying for the fan-out. Refs: WOO-589 Co-Authored-By: Claude Opus 5.5 (1M context) --- lib/AppInfo/Application.php | 2 + .../ModuleVersionPublicationListener.php | 26 ++- .../ModuleVersionPublicationService.php | 178 +++++++++++++++--- .../OpenRegister/Event/ObjectDeletedEvent.php | 57 ++++++ .../OpenRegister/Event/ObjectUpdatedEvent.php | 84 +++++++++ .../ModuleVersionPublicationServiceTest.php | 128 ++++++++++++- 6 files changed, 432 insertions(+), 43 deletions(-) create mode 100644 tests/Stubs/OpenRegister/Event/ObjectDeletedEvent.php create mode 100644 tests/Stubs/OpenRegister/Event/ObjectUpdatedEvent.php diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 7e31c1f7..a185d5a5 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -22,6 +22,7 @@ use OCA\OpenRegister\Contract\ObjectServiceInterface; use OCA\OpenRegister\Event\ObjectCreatedEvent; +use OCA\OpenRegister\Event\ObjectDeletedEvent; use OCA\OpenRegister\Event\ObjectUpdatedEvent; use OCA\OpenRegister\Event\UserProfileUpdatedEvent; use OCA\OpenRegister\Service\OrganisationService as OpenRegisterOrganisationService; @@ -815,6 +816,7 @@ private function registerEventListeners(IRegistrationContext $context): void { // A module version is public only while its application is (publication-field-rules). $context->registerEventListener(ObjectCreatedEvent::class, ModuleVersionPublicationListener::class); $context->registerEventListener(ObjectUpdatedEvent::class, ModuleVersionPublicationListener::class); + $context->registerEventListener(ObjectDeletedEvent::class, ModuleVersionPublicationListener::class); // Sync user profile updates into the contactpersoon mirror. $context->registerEventListener(UserProfileUpdatedEvent::class, UserProfileUpdatedEventListener::class); diff --git a/lib/EventListener/ModuleVersionPublicationListener.php b/lib/EventListener/ModuleVersionPublicationListener.php index 581ba733..1e143b51 100644 --- a/lib/EventListener/ModuleVersionPublicationListener.php +++ b/lib/EventListener/ModuleVersionPublicationListener.php @@ -21,6 +21,7 @@ namespace OCA\Stackiq\EventListener; use OCA\OpenRegister\Event\ObjectCreatedEvent; +use OCA\OpenRegister\Event\ObjectDeletedEvent; use OCA\OpenRegister\Event\ObjectUpdatedEvent; use OCA\Stackiq\Service\ModuleVersionPublicationService; use OCP\EventDispatcher\Event; @@ -29,7 +30,7 @@ use Throwable; /** - * Hands every created or updated object to the publication mirror. + * Hands every created, updated or deleted object to the publication mirror. * * @spec openspec/changes/publication-field-rules/specs/publication-field-rules/spec.md#requirement-req-pfr-002-a-module-version-is-public-only-while-its-application-is * @@ -59,21 +60,18 @@ public function __construct( * @spec openspec/changes/publication-field-rules/specs/publication-field-rules/spec.md#requirement-req-pfr-002-a-module-version-is-public-only-while-its-application-is */ public function handle(Event $event): void { - $object = null; - if ($event instanceof ObjectUpdatedEvent) { - $object = $event->getNewObject(); - } - - if ($event instanceof ObjectCreatedEvent) { - $object = $event->getObject(); - } + try { + if ($event instanceof ObjectUpdatedEvent) { + $this->publication->objectSaved(object: $event->getNewObject(), previous: $event->getOldObject()); + } - if ($object === null) { - return; - } + if ($event instanceof ObjectCreatedEvent) { + $this->publication->objectSaved(object: $event->getObject()); + } - try { - $this->publication->objectSaved(object: $object); + if ($event instanceof ObjectDeletedEvent) { + $this->publication->objectDeleted(object: $event->getObject()); + } } catch (Throwable $e) { $this->logger->error('ModuleVersionPublicationListener: could not mirror the publication', ['error' => $e->getMessage()]); } diff --git a/lib/Service/ModuleVersionPublicationService.php b/lib/Service/ModuleVersionPublicationService.php index 0126dfb5..3677b1a6 100644 --- a/lib/Service/ModuleVersionPublicationService.php +++ b/lib/Service/ModuleVersionPublicationService.php @@ -42,7 +42,7 @@ class ModuleVersionPublicationService { /** - * The most versions one module save updates. + * How many versions one search reads; a module with more is read page by page. * * @var integer */ @@ -96,15 +96,25 @@ private static function text(mixed $value): ?string { /** * React to a saved object: a module updates its versions, a version reads its module. * - * @param ObjectEntityInterface $object The saved object. + * A module update that leaves its publication date and registrant as they + * were has nothing to copy, so its versions are not searched. + * + * @param ObjectEntityInterface $object The saved object. + * @param ObjectEntityInterface|null $previous The object before an update, or null for a new one. * * @return integer The number of versions written. * * @spec openspec/changes/publication-field-rules/specs/publication-field-rules/spec.md#requirement-req-pfr-002-a-module-version-is-public-only-while-its-application-is */ - public function objectSaved(ObjectEntityInterface $object): int { + public function objectSaved(ObjectEntityInterface $object, ?ObjectEntityInterface $previous = null): int { $schema = (string) $object->getSchema(); if ($schema === (string) $this->settingsService->getSchemaIdForObjectType('module')) { + if ($previous !== null + && self::mirrorOf(module: (array) $previous->getObject()) === self::mirrorOf(module: (array) $object->getObject()) + ) { + return 0; + } + return $this->moduleSaved(module: $object); } @@ -115,6 +125,23 @@ public function objectSaved(ObjectEntityInterface $object): int { return 0; }//end objectSaved() + /** + * React to a deleted object: the versions of a deleted module stop following a publication. + * + * @param ObjectEntityInterface $object The deleted object. + * + * @return integer The number of versions written. + * + * @spec openspec/changes/publication-field-rules/specs/publication-field-rules/spec.md#requirement-req-pfr-002-a-module-version-is-public-only-while-its-application-is + */ + public function objectDeleted(ObjectEntityInterface $object): int { + if ((string) $object->getSchema() !== (string) $this->settingsService->getSchemaIdForObjectType('module')) { + return 0; + } + + return $this->copyOntoVersions(moduleUuid: (string) $object->getUuid(), mirror: self::mirrorOf(module: []))['written']; + }//end objectDeleted() + /** * Copy a module's publication onto every version of it that differs. * @@ -125,34 +152,123 @@ public function objectSaved(ObjectEntityInterface $object): int { * @spec openspec/changes/publication-field-rules/specs/publication-field-rules/spec.md#requirement-req-pfr-002-a-module-version-is-public-only-while-its-application-is */ public function moduleSaved(ObjectEntityInterface $module): int { + return $this->backfillModule(module: $module)['written']; + }//end moduleSaved() + + /** + * Copy a module's publication onto its versions and say what could not be copied. + * + * @param ObjectEntityInterface $module The module. + * + * @return array{written: int, failed: int} The versions written, and the versions or searches that failed. + * + * @spec openspec/changes/publication-field-rules/specs/publication-field-rules/spec.md#requirement-req-pfr-002-a-module-version-is-public-only-while-its-application-is + */ + public function backfillModule(ObjectEntityInterface $module): array { + return $this->copyOntoVersions(moduleUuid: (string) $module->getUuid(), mirror: self::mirrorOf(module: (array) $module->getObject())); + }//end backfillModule() + + /** + * Copy a mirror onto every version of a module that differs, page by page. + * + * @param string $moduleUuid The module. + * @param array{modulePublicationDate: string|null, moduleRegisteredBy: string|null} $mirror The values to hold. + * + * @return array{written: int, failed: int} The versions written, and the versions or searches that failed. + */ + private function copyOntoVersions(string $moduleUuid, array $mirror): array { $objects = $this->objectService(); $register = $this->settingsService->getRegisterIdForObjectType('moduleVersion'); $schema = $this->settingsService->getSchemaIdForObjectType('moduleVersion'); + $result = ['written' => 0, 'failed' => 0]; if ($objects === null || $register === null || $schema === null) { - return 0; + return $result; } - try { - $versions = $objects->searchObjects( - query: ['register' => $register, 'schema' => $schema, 'module' => $module->getUuid(), '_limit' => self::VERSION_LIMIT], - _rbac: false, - _multitenancy: false - ); - } catch (Throwable $e) { - $this->logger->error('ModuleVersionPublicationService: could not read the versions', ['error' => $e->getMessage()]); - return 0; - } + $offset = 0; + do { + try { + $versions = (array) $objects->searchObjects( + query: [ + 'register' => $register, + 'schema' => $schema, + 'module' => $moduleUuid, + '_limit' => self::VERSION_LIMIT, + '_offset' => $offset, + ], + _rbac: false, + _multitenancy: false + ); + } catch (Throwable $e) { + $this->logFailure( + message: 'ModuleVersionPublicationService: could not read the versions', + context: ['module' => $moduleUuid, 'error' => $e->getMessage()], + depublishes: (self::isPublicNow(mirror: $mirror) === false) + ); + $result['failed']++; + return $result; + } + + foreach ($versions as $version) { + if (($version instanceof ObjectEntityInterface) === false) { + continue; + } - $mirror = self::mirrorOf(module: (array) $module->getObject()); - $written = 0; - foreach ((array) $versions as $version) { - if (($version instanceof ObjectEntityInterface) === true && $this->write(objects: $objects, version: $version, mirror: $mirror) === true) { - $written++; + $outcome = $this->write(objects: $objects, version: $version, mirror: $mirror); + if ($outcome === true) { + $result['written']++; + } + + if ($outcome === null) { + $result['failed']++; + } } + + $offset += self::VERSION_LIMIT; + } while (count($versions) === self::VERSION_LIMIT); + + return $result; + }//end copyOntoVersions() + + /** + * Whether a version holding this mirror is public now, by the moduleVersion read rule. + * + * @param array $mirror The mirrored fields. + * + * @return boolean True when an anonymous reader may read it. + */ + private static function isPublicNow(array $mirror): bool { + if (($mirror['moduleRegisteredBy'] ?? null) === 'Supplier') { + return true; } - return $written; - }//end moduleSaved() + $date = ($mirror['modulePublicationDate'] ?? null); + if (is_string($date) === false || $date === '') { + return false; + } + + $time = strtotime($date); + + return $time !== false && $time <= time(); + }//end isPublicNow() + + /** + * Log a mirror that could not be written: critical when it leaves a version public that should not be. + * + * @param string $message The message. + * @param array $context The context. + * @param boolean $depublishes Whether the write would have taken a version out of public view. + * + * @return void + */ + private function logFailure(string $message, array $context, bool $depublishes): void { + if ($depublishes === true) { + $this->logger->critical($message . '; the version stays public until it is saved again or the backfill runs', $context); + return; + } + + $this->logger->error($message, $context); + }//end logFailure() /** * Copy the module's publication onto a saved version, when it differs. @@ -216,9 +332,13 @@ private static function referenceOf(mixed $value): ?string { * @param ObjectEntityInterface $version The version. * @param array{modulePublicationDate: string|null, moduleRegisteredBy: string|null} $mirror The values to hold. * - * @return boolean True when it was written. + * The version is saved without validation: only the two mirrored fields + * change, and a version holding older data the schema no longer accepts + * must still follow its module. + * + * @return boolean|null True when it was written, false when it was in step, null when the write failed. */ - private function write(ObjectServiceInterface $objects, ObjectEntityInterface $version, array $mirror): bool { + private function write(ObjectServiceInterface $objects, ObjectEntityInterface $version, array $mirror): ?bool { $data = (array) $version->getObject(); if (($data['modulePublicationDate'] ?? null) === $mirror['modulePublicationDate'] && ($data['moduleRegisteredBy'] ?? null) === $mirror['moduleRegisteredBy'] @@ -234,14 +354,16 @@ private function write(ObjectServiceInterface $objects, ObjectEntityInterface $v schema: $version->getSchema(), uuid: $version->getUuid(), _rbac: false, - _multitenancy: false + _multitenancy: false, + _validation: false ); } catch (Throwable $e) { - $this->logger->error( - 'ModuleVersionPublicationService: could not copy the publication onto a version', - ['uuid' => $version->getUuid(), 'error' => $e->getMessage()] + $this->logFailure( + message: 'ModuleVersionPublicationService: could not copy the publication onto a version', + context: ['uuid' => $version->getUuid(), 'error' => $e->getMessage()], + depublishes: (self::isPublicNow(mirror: $data) === true && self::isPublicNow(mirror: $mirror) === false) ); - return false; + return null; } return true; diff --git a/tests/Stubs/OpenRegister/Event/ObjectDeletedEvent.php b/tests/Stubs/OpenRegister/Event/ObjectDeletedEvent.php new file mode 100644 index 00000000..08eb78d3 --- /dev/null +++ b/tests/Stubs/OpenRegister/Event/ObjectDeletedEvent.php @@ -0,0 +1,57 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @version GIT: + * @link https://github.com/ConductionNL/stackiq + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Event; + +use OCA\OpenRegister\Db\ObjectEntity; +use OCP\EventDispatcher\Event; + +/** + * Dispatched after an object is deleted. + */ +class ObjectDeletedEvent extends Event { + + /** + * The deleted object. + * + * @var ObjectEntity + */ + private ObjectEntity $object; + + /** + * Constructor. + * + * @param ObjectEntity $object The deleted object. + */ + public function __construct(ObjectEntity $object) { + parent::__construct(); + $this->object = $object; + }//end __construct() + + /** + * The deleted object. + * + * @return ObjectEntity The object. + */ + public function getObject(): ObjectEntity { + return $this->object; + }//end getObject() +}//end class diff --git a/tests/Stubs/OpenRegister/Event/ObjectUpdatedEvent.php b/tests/Stubs/OpenRegister/Event/ObjectUpdatedEvent.php new file mode 100644 index 00000000..f74aca7f --- /dev/null +++ b/tests/Stubs/OpenRegister/Event/ObjectUpdatedEvent.php @@ -0,0 +1,84 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @version GIT: + * @link https://github.com/ConductionNL/stackiq + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Event; + +use OCA\OpenRegister\Db\ObjectEntity; +use OCP\EventDispatcher\Event; + +/** + * Dispatched after an object is updated. + */ +class ObjectUpdatedEvent extends Event { + + /** + * The object after the update. + * + * @var ObjectEntity + */ + private ObjectEntity $newObject; + + /** + * The object before the update, when known. + * + * @var ObjectEntity|null + */ + private ?ObjectEntity $oldObject; + + /** + * Constructor. + * + * @param ObjectEntity $newObject The object after the update. + * @param ObjectEntity|null $oldObject The object before the update, when known. + */ + public function __construct(ObjectEntity $newObject, ?ObjectEntity $oldObject = null) { + parent::__construct(); + $this->newObject = $newObject; + $this->oldObject = $oldObject; + }//end __construct() + + /** + * The object after the update. + * + * @return ObjectEntity The object. + */ + public function getObject(): ObjectEntity { + return $this->newObject; + }//end getObject() + + /** + * The object after the update. + * + * @return ObjectEntity The object. + */ + public function getNewObject(): ObjectEntity { + return $this->newObject; + }//end getNewObject() + + /** + * The object before the update. + * + * @return ObjectEntity|null The object, or null when it is not known. + */ + public function getOldObject(): ?ObjectEntity { + return $this->oldObject; + }//end getOldObject() +}//end class diff --git a/tests/Unit/Service/ModuleVersionPublicationServiceTest.php b/tests/Unit/Service/ModuleVersionPublicationServiceTest.php index 6795db18..d111af0f 100644 --- a/tests/Unit/Service/ModuleVersionPublicationServiceTest.php +++ b/tests/Unit/Service/ModuleVersionPublicationServiceTest.php @@ -21,6 +21,8 @@ use OCA\OpenRegister\Contract\ObjectEntityInterface; use OCA\OpenRegister\Contract\ObjectServiceInterface; use OCA\OpenRegister\Event\ObjectCreatedEvent; +use OCA\OpenRegister\Event\ObjectDeletedEvent; +use OCA\OpenRegister\Event\ObjectUpdatedEvent; use OCA\Stackiq\EventListener\ModuleVersionPublicationListener; use OCA\Stackiq\Service\ModuleVersionPublicationService; use OCA\Stackiq\Service\SettingsService; @@ -41,6 +43,13 @@ class ModuleVersionPublicationServiceTest extends TestCase { */ private ObjectServiceInterface&MockObject $objects; + /** + * The logger double of the current service. + * + * @var LoggerInterface&MockObject + */ + private LoggerInterface&MockObject $logger; + /** * An object with the six accessors of OpenRegister's entity contract. * @@ -131,7 +140,9 @@ private function service(): ModuleVersionPublicationService { $container = $this->createMock(ContainerInterface::class); $container->method('get')->willReturn($this->objects); - return new ModuleVersionPublicationService(settingsService: $settings, container: $container, logger: $this->createMock(LoggerInterface::class)); + $this->logger = $this->createMock(LoggerInterface::class); + + return new ModuleVersionPublicationService(settingsService: $settings, container: $container, logger: $this->logger); }//end service() /** @@ -159,6 +170,7 @@ function (...$args) use (&$written, $stale) { $this->assertSame('2026-09-01T00:00:00+00:00', $written[0][0]['modulePublicationDate']); $this->assertSame('Municipality', $written[0][0]['moduleRegisteredBy']); $this->assertSame('1.0', $written[0][0]['version'], 'the version keeps its own data'); + $this->assertFalse($written[0][8], 'saved without validation, so older version data never blocks the mirror'); }//end testAPublishedModuleReachesTheVersionsThatDiffer() /** @@ -189,6 +201,120 @@ public function testAVersionReadsItsModuleAndStopsWhenInStep(): void { $this->assertSame(0, $service->objectSaved(object: self::entity('x-1', '99', ['module' => 'm-1'])), 'other schemas are ignored'); }//end testAVersionReadsItsModuleAndStopsWhenInStep() + /** + * A module with more versions than one search returns is read page by page, so every version follows it. + * + * @return void + */ + public function testAModuleWithManyVersionsIsReadPageByPage(): void { + $service = $this->service(); + $page = []; + for ($i = 0; $i < ModuleVersionPublicationService::VERSION_LIMIT; $i++) { + $page[] = self::entity('v-' . $i, '46', ['module' => 'm-1']); + } + + $offsets = []; + $this->objects->method('searchObjects')->willReturnCallback( + static function (array $query) use (&$offsets, $page): array { + $offsets[] = $query['_offset']; + return match ($query['_offset']) { + 0 => $page, + ModuleVersionPublicationService::VERSION_LIMIT => [self::entity('v-last', '46', ['module' => 'm-1'])], + default => [], + }; + } + ); + $this->objects->expects($this->exactly(ModuleVersionPublicationService::VERSION_LIMIT + 1))->method('saveObject')->willReturn($page[0]); + + $written = $service->objectSaved(object: self::entity('m-1', '43', ['registeredBy' => 'Supplier'])); + + $this->assertSame(ModuleVersionPublicationService::VERSION_LIMIT + 1, $written); + $this->assertSame([0, ModuleVersionPublicationService::VERSION_LIMIT], $offsets); + }//end testAModuleWithManyVersionsIsReadPageByPage() + + /** + * A module update that leaves its publication as it was does not search its versions. + * + * @return void + */ + public function testAModuleUpdateWithoutAPublicationChangeLeavesTheVersions(): void { + $service = $this->service(); + $this->objects->expects($this->never())->method('searchObjects'); + + $before = self::entity('m-1', '43', ['name' => 'Zaaksysteem', 'publicationDate' => '2026-09-01T00:00:00+00:00', 'registeredBy' => 'Municipality']); + $after = self::entity('m-1', '43', ['name' => 'Zaaksysteem 2', 'publicationDate' => '2026-09-01T00:00:00+00:00', 'registeredBy' => 'Municipality']); + + $this->assertSame(0, $service->objectSaved(object: $after, previous: $before)); + }//end testAModuleUpdateWithoutAPublicationChangeLeavesTheVersions() + + /** + * Deleting a module clears its versions, so they stop being public. + * + * @return void + */ + public function testADeletedModuleClearsItsVersions(): void { + $service = $this->service(); + $this->objects->method('searchObjects')->willReturn([self::entity('v-1', '46', ['module' => 'm-1', 'moduleRegisteredBy' => 'Supplier'])]); + $this->objects->expects($this->once())->method('saveObject')->with( + $this->callback(static fn (array $data): bool => $data['modulePublicationDate'] === null && $data['moduleRegisteredBy'] === null) + ); + + $this->assertSame(1, $service->objectDeleted(object: self::entity('m-1', '43', ['registeredBy' => 'Supplier']))); + $this->assertSame(0, $service->objectDeleted(object: self::entity('v-1', '46', ['module' => 'm-1'])), 'only a module clears versions'); + }//end testADeletedModuleClearsItsVersions() + + /** + * A depublication that cannot be written is logged as critical: the version stays public. + * + * @return void + */ + public function testAFailedDepublicationIsCritical(): void { + $service = $this->service(); + $this->objects->method('searchObjects')->willReturn([self::entity('v-1', '46', ['module' => 'm-1', 'modulePublicationDate' => '2026-09-01T00:00:00+00:00', 'moduleRegisteredBy' => 'Municipality'])]); + $this->objects->method('saveObject')->willThrowException(new \RuntimeException('database went away')); + $this->logger->expects($this->once())->method('critical')->with($this->stringContains('stays public')); + $this->logger->expects($this->never())->method('error'); + + $this->assertSame(0, $service->objectSaved(object: self::entity('m-1', '43', ['registeredBy' => 'Municipality']))); + }//end testAFailedDepublicationIsCritical() + + /** + * The backfill hears how many versions or searches failed, so it can tell a full pass from a partial one. + * + * @return void + */ + public function testTheBackfillCountsFailures(): void { + $service = $this->service(); + $this->objects->method('searchObjects')->willThrowException(new \RuntimeException('index offline')); + + $this->assertSame(['written' => 0, 'failed' => 1], $service->backfillModule(module: self::entity('m-1', '43', ['registeredBy' => 'Supplier']))); + }//end testTheBackfillCountsFailures() + + /** + * The listener hands an update with the object before it, and a delete, to the service. + * + * @return void + */ + public function testTheListenerPassesUpdatesAndDeletes(): void { + $service = $this->getMockBuilder(ModuleVersionPublicationService::class) + ->disableOriginalConstructor() + ->onlyMethods(['objectSaved', 'objectDeleted']) + ->getMock(); + $new = $this->getMockBuilder(\OCA\OpenRegister\Db\ObjectEntity::class)->disableOriginalConstructor()->getMockForAbstractClass(); + $old = $this->getMockBuilder(\OCA\OpenRegister\Db\ObjectEntity::class)->disableOriginalConstructor()->getMockForAbstractClass(); + $service->expects($this->once())->method('objectSaved')->with($new, $old)->willReturn(0); + $service->expects($this->once())->method('objectDeleted')->with($old)->willReturn(0); + + $listener = new ModuleVersionPublicationListener(publication: $service, logger: $this->createMock(LoggerInterface::class)); + $listener->handle(new ObjectUpdatedEvent($new, $old)); + $listener->handle(new ObjectDeletedEvent($old)); + + $this->assertStringContainsString( + 'registerEventListener(ObjectDeletedEvent::class, ModuleVersionPublicationListener::class)', + (string) file_get_contents(__DIR__ . '/../../../lib/AppInfo/Application.php') + ); + }//end testTheListenerPassesUpdatesAndDeletes() + /** * The listener hands a created object to the service: the wiring from the caller. * From 9d0921aae311a703ec98bf9efcc0a1758169b3cf Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Fri, 2 Oct 2026 15:32:32 +0200 Subject: [PATCH 06/10] fix(repair): the module version publication backfill pages through modules and runs once The backfill read every module in one unbounded findAll() on each `occ upgrade`. It now reads modules in pages of 200 and, after a pass in which no version or search failed, sets the app-config flag `module_version_publication_backfilled` so later upgrades skip it. A pass with failures is not recorded and runs again on the next upgrade. Refs: WOO-589 Co-Authored-By: Claude Opus 5.5 (1M context) --- .../BackfillModuleVersionPublication.php | 66 ++++++-- .../BackfillModuleVersionPublicationTest.php | 160 ++++++++++++++++++ 2 files changed, 214 insertions(+), 12 deletions(-) create mode 100644 tests/Unit/Repair/BackfillModuleVersionPublicationTest.php diff --git a/lib/Repair/BackfillModuleVersionPublication.php b/lib/Repair/BackfillModuleVersionPublication.php index 9a0b9b28..0a38739a 100644 --- a/lib/Repair/BackfillModuleVersionPublication.php +++ b/lib/Repair/BackfillModuleVersionPublication.php @@ -7,7 +7,9 @@ * so for anonymous readers they read as unpublished until their module is * saved again. That is the safe direction; this step makes the published ones * public again without waiting for an edit. It is idempotent: a version that - * already holds its module's values is not written. + * already holds its module's values is not written. It reads the modules page + * by page, and after a pass in which nothing failed it records that in the app + * config, so later upgrades skip it. * * @category Repair * @package OCA\Stackiq\Repair @@ -27,9 +29,11 @@ namespace OCA\Stackiq\Repair; use OCA\OpenRegister\Contract\ObjectEntityInterface; +use OCA\Stackiq\AppInfo\Application; use OCA\Stackiq\Service\ModuleVersionPublicationService; use OCA\Stackiq\Service\SettingsService; use OCP\App\IAppManager; +use OCP\IAppConfig; use OCP\Migration\IOutput; use OCP\Migration\IRepairStep; use Throwable; @@ -41,17 +45,33 @@ */ class BackfillModuleVersionPublication implements IRepairStep { + /** + * App-config key set after a pass in which every module and version was handled. + * + * @var string + */ + public const DONE_CONFIG_KEY = 'module_version_publication_backfilled'; + + /** + * How many modules one read returns. + * + * @var integer + */ + public const PAGE_SIZE = 200; + /** * Constructor. * * @param IAppManager $appManager Tells whether OpenRegister is installed. * @param SettingsService $settingsService Resolves the module schema and the object service. * @param ModuleVersionPublicationService $publication The mirror. + * @param IAppConfig $appConfig Holds the done marker. */ public function __construct( private readonly IAppManager $appManager, private readonly SettingsService $settingsService, private readonly ModuleVersionPublicationService $publication, + private readonly IAppConfig $appConfig, ) { }//end __construct() @@ -74,6 +94,10 @@ public function getName(): string { * @spec openspec/changes/publication-field-rules/specs/publication-field-rules/spec.md#requirement-req-pfr-002-a-module-version-is-public-only-while-its-application-is */ public function run(IOutput $output): void { + if ($this->appConfig->getValueBool(Application::APP_ID, self::DONE_CONFIG_KEY, false) === true) { + return; + } + if (in_array('openregister', $this->appManager->getInstalledApps(), true) === false) { $output->info('OpenRegister not installed, so there are no versions to update.'); return; @@ -87,20 +111,38 @@ public function run(IOutput $output): void { return; } - try { - $modules = $objects->setRegister($register)->setSchema($schema)->findAll([], false, false); - } catch (Throwable $e) { - $output->warning('Could not read the applications: ' . $e->getMessage()); - return; - } - $written = 0; - foreach ((array) $modules as $module) { - if (($module instanceof ObjectEntityInterface) === true) { - $written += $this->publication->moduleSaved(module: $module); + $failed = 0; + $offset = 0; + do { + try { + $modules = (array) $objects->setRegister($register)->setSchema($schema)->findAll( + ['limit' => self::PAGE_SIZE, 'offset' => $offset], + false, + false + ); + } catch (Throwable $e) { + $output->warning('Could not read the applications: ' . $e->getMessage()); + return; } - } + + foreach ($modules as $module) { + if (($module instanceof ObjectEntityInterface) === true) { + $result = $this->publication->backfillModule(module: $module); + $written += $result['written']; + $failed += $result['failed']; + } + } + + $offset += self::PAGE_SIZE; + } while (count($modules) === self::PAGE_SIZE); $output->info($written . ' versions now follow their application\'s publication.'); + if ($failed > 0) { + $output->warning($failed . ' versions or searches failed; the next upgrade tries again.'); + return; + } + + $this->appConfig->setValueBool(Application::APP_ID, self::DONE_CONFIG_KEY, true); }//end run() }//end class diff --git a/tests/Unit/Repair/BackfillModuleVersionPublicationTest.php b/tests/Unit/Repair/BackfillModuleVersionPublicationTest.php new file mode 100644 index 00000000..d95466ba --- /dev/null +++ b/tests/Unit/Repair/BackfillModuleVersionPublicationTest.php @@ -0,0 +1,160 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://github.com/ConductionNL/stackiq + * + * @spec openspec/changes/publication-field-rules/specs/publication-field-rules/spec.md#requirement-req-pfr-002-a-module-version-is-public-only-while-its-application-is + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Stackiq\Tests\Unit\Repair; + +use OCA\OpenRegister\Contract\ObjectEntityInterface; +use OCA\OpenRegister\Contract\ObjectServiceInterface; +use OCA\Stackiq\Repair\BackfillModuleVersionPublication; +use OCA\Stackiq\Service\ModuleVersionPublicationService; +use OCA\Stackiq\Service\SettingsService; +use OCP\App\IAppManager; +use OCP\IAppConfig; +use OCP\Migration\IOutput; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; + +/** + * Pages, the done marker, and a failed pass that runs again. + */ +class BackfillModuleVersionPublicationTest extends TestCase { + + /** + * The app settings, by key. + * + * @var array + */ + private array $settings = []; + + /** + * The object service double. + * + * @var ObjectServiceInterface&MockObject + */ + private ObjectServiceInterface&MockObject $objects; + + /** + * The mirror double. + * + * @var ModuleVersionPublicationService&MockObject + */ + private ModuleVersionPublicationService&MockObject $publication; + + /** + * The step under test, with OpenRegister installed and the module schema configured. + * + * @return BackfillModuleVersionPublication + */ + private function step(): BackfillModuleVersionPublication { + $apps = $this->createMock(IAppManager::class); + $apps->method('getInstalledApps')->willReturn(['openregister', 'stackiq']); + + $this->objects = $this->createMock(ObjectServiceInterface::class); + $this->objects->method('setRegister')->willReturnSelf(); + $this->objects->method('setSchema')->willReturnSelf(); + + $settings = $this->getMockBuilder(SettingsService::class) + ->disableOriginalConstructor() + ->onlyMethods(['getRegisterIdForObjectType', 'getSchemaIdForObjectType', 'getObjectService']) + ->getMock(); + $settings->method('getRegisterIdForObjectType')->willReturn(20); + $settings->method('getSchemaIdForObjectType')->willReturn(43); + $settings->method('getObjectService')->willReturn($this->objects); + + $this->publication = $this->getMockBuilder(ModuleVersionPublicationService::class) + ->disableOriginalConstructor() + ->onlyMethods(['backfillModule']) + ->getMock(); + + $config = $this->createMock(IAppConfig::class); + $config->method('getValueBool')->willReturnCallback(fn (string $app, string $key, bool $default = false): bool => (bool) ($this->settings[$key] ?? $default)); + $config->method('setValueBool')->willReturnCallback( + function (string $app, string $key, bool $value): bool { + $this->settings[$key] = $value; + return true; + } + ); + + return new BackfillModuleVersionPublication(appManager: $apps, settingsService: $settings, publication: $this->publication, appConfig: $config); + }//end step() + + /** + * Modules are read in pages until a short page, and a pass without failures sets the marker. + * + * @return void + */ + public function testModulesAreReadPageByPageAndAFullPassIsRecorded(): void { + $step = $this->step(); + $module = $this->createMock(ObjectEntityInterface::class); + $full = array_fill(0, BackfillModuleVersionPublication::PAGE_SIZE, $module); + $pages = []; + $this->objects->method('findAll')->willReturnCallback( + static function (array $config) use (&$pages, $full, $module): array { + $pages[] = $config; + if ($config['offset'] === 0) { + return $full; + } + + return [$module]; + } + ); + $this->publication->expects($this->exactly(BackfillModuleVersionPublication::PAGE_SIZE + 1)) + ->method('backfillModule')->willReturn(['written' => 1, 'failed' => 0]); + + $step->run($this->createMock(IOutput::class)); + + $this->assertSame( + [ + ['limit' => BackfillModuleVersionPublication::PAGE_SIZE, 'offset' => 0], + ['limit' => BackfillModuleVersionPublication::PAGE_SIZE, 'offset' => BackfillModuleVersionPublication::PAGE_SIZE], + ], + $pages + ); + $this->assertTrue($this->settings[BackfillModuleVersionPublication::DONE_CONFIG_KEY]); + }//end testModulesAreReadPageByPageAndAFullPassIsRecorded() + + /** + * After a recorded pass the step reads nothing. + * + * @return void + */ + public function testARecordedPassIsSkipped(): void { + $step = $this->step(); + $this->settings[BackfillModuleVersionPublication::DONE_CONFIG_KEY] = true; + $this->objects->expects($this->never())->method('findAll'); + + $step->run($this->createMock(IOutput::class)); + }//end testARecordedPassIsSkipped() + + /** + * A pass in which a version failed is not recorded, so the next upgrade tries again. + * + * @return void + */ + public function testAPassWithFailuresRunsAgain(): void { + $step = $this->step(); + $this->objects->method('findAll')->willReturn([$this->createMock(ObjectEntityInterface::class)]); + $this->publication->method('backfillModule')->willReturn(['written' => 0, 'failed' => 1]); + + $step->run($this->createMock(IOutput::class)); + + $this->assertArrayNotHasKey(BackfillModuleVersionPublication::DONE_CONFIG_KEY, $this->settings); + }//end testAPassWithFailuresRunsAgain() +}//end class From 9543e63288fc4708b1da78599b332b35aa2e27da Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Fri, 2 Oct 2026 15:40:34 +0200 Subject: [PATCH 07/10] fix(itsm): the service desk pages say when a request fails instead of showing nothing The exchange settings section loaded its organisations with a raw `/index.php/apps/...` fetch and hard-coded register and schema slugs, which breaks under a sub-path, and turned a failed call into an empty list. It now reads the configured register and schema from `/api/voorzieningen/config` and calls OpenRegister through `@nextcloud/axios` and `generateUrl()`, as the CMDB import does, and shows a note when the organisations cannot be loaded. The CMDB page catches a failed file upload or a non-JSON answer (a proxy error page) and says the file could not be sent, and shows a separate "could not be loaded" note when the exchange status call fails, instead of claiming no service desk is connected. Refs: WOO-589 Co-Authored-By: Claude Opus 5.5 (1M context) --- l10n/en.js | 6 ++- l10n/en.json | 6 ++- l10n/nl.js | 6 ++- l10n/nl.json | 6 ++- src/views/cmdb/CmdbOverview.vue | 27 +++++++++++-- src/views/settings/sections/ItsmExchange.vue | 42 +++++++++++++++----- 6 files changed, 77 insertions(+), 16 deletions(-) diff --git a/l10n/en.js b/l10n/en.js index 579aecf1..1f8c6f9a 100644 --- a/l10n/en.js +++ b/l10n/en.js @@ -1073,7 +1073,11 @@ OC.L10N.register( "Application published": "Application published", "The publication date of the application this version belongs to, copied from it. A version is public only while its application is.": "The publication date of the application this version belongs to, copied from it. A version is public only while its application is.", "Application registered by": "Application registered by", - "Who registered the application this version belongs to, copied from it. A supplier's application is public, and so are its versions.": "Who registered the application this version belongs to, copied from it. A supplier's application is public, and so are its versions." + "Who registered the application this version belongs to, copied from it. A supplier's application is public, and so are its versions.": "Who registered the application this version belongs to, copied from it. A supplier's application is public, and so are its versions.", + "The file could not be sent. Check your connection and try again.": "The file could not be sent. Check your connection and try again.", + "The organisation register is not configured, so there are no organisations to choose from.": "The organisation register is not configured, so there are no organisations to choose from.", + "The organisations could not be loaded. Reload the page to try again.": "The organisations could not be loaded. Reload the page to try again.", + "Whether a service desk is connected could not be loaded. Reload the page to try again.": "Whether a service desk is connected could not be loaded. Reload the page to try again." }, "nplurals=2; plural=(n != 1);" ) diff --git a/l10n/en.json b/l10n/en.json index 494792fa..800f5f8b 100644 --- a/l10n/en.json +++ b/l10n/en.json @@ -1072,6 +1072,10 @@ "Application published": "Application published", "The publication date of the application this version belongs to, copied from it. A version is public only while its application is.": "The publication date of the application this version belongs to, copied from it. A version is public only while its application is.", "Application registered by": "Application registered by", - "Who registered the application this version belongs to, copied from it. A supplier's application is public, and so are its versions.": "Who registered the application this version belongs to, copied from it. A supplier's application is public, and so are its versions." + "Who registered the application this version belongs to, copied from it. A supplier's application is public, and so are its versions.": "Who registered the application this version belongs to, copied from it. A supplier's application is public, and so are its versions.", + "The file could not be sent. Check your connection and try again.": "The file could not be sent. Check your connection and try again.", + "The organisation register is not configured, so there are no organisations to choose from.": "The organisation register is not configured, so there are no organisations to choose from.", + "The organisations could not be loaded. Reload the page to try again.": "The organisations could not be loaded. Reload the page to try again.", + "Whether a service desk is connected could not be loaded. Reload the page to try again.": "Whether a service desk is connected could not be loaded. Reload the page to try again." } } diff --git a/l10n/nl.js b/l10n/nl.js index 77d23a5e..5b3007be 100644 --- a/l10n/nl.js +++ b/l10n/nl.js @@ -1143,7 +1143,11 @@ OC.L10N.register( "Application published": "Applicatie gepubliceerd", "The publication date of the application this version belongs to, copied from it. A version is public only while its application is.": "De publicatiedatum van de applicatie waar deze versie bij hoort, daarvan overgenomen. Een versie is alleen openbaar zolang haar applicatie dat is.", "Application registered by": "Applicatie geregistreerd door", - "Who registered the application this version belongs to, copied from it. A supplier's application is public, and so are its versions.": "Wie de applicatie registreerde waar deze versie bij hoort, daarvan overgenomen. De applicatie van een leverancier is openbaar, en haar versies ook." + "Who registered the application this version belongs to, copied from it. A supplier's application is public, and so are its versions.": "Wie de applicatie registreerde waar deze versie bij hoort, daarvan overgenomen. De applicatie van een leverancier is openbaar, en haar versies ook.", + "The file could not be sent. Check your connection and try again.": "Het bestand kon niet worden verstuurd. Controleer uw verbinding en probeer het opnieuw.", + "The organisation register is not configured, so there are no organisations to choose from.": "Het organisatieregister is niet ingesteld, dus er zijn geen organisaties om uit te kiezen.", + "The organisations could not be loaded. Reload the page to try again.": "De organisaties konden niet worden geladen. Herlaad de pagina om het opnieuw te proberen.", + "Whether a service desk is connected could not be loaded. Reload the page to try again.": "Kon niet worden geladen of er een servicedesk is gekoppeld. Herlaad de pagina om het opnieuw te proberen." }, "nplurals=2; plural=(n != 1);" ) diff --git a/l10n/nl.json b/l10n/nl.json index 7de3e961..23f291cf 100644 --- a/l10n/nl.json +++ b/l10n/nl.json @@ -1142,6 +1142,10 @@ "Application published": "Applicatie gepubliceerd", "The publication date of the application this version belongs to, copied from it. A version is public only while its application is.": "De publicatiedatum van de applicatie waar deze versie bij hoort, daarvan overgenomen. Een versie is alleen openbaar zolang haar applicatie dat is.", "Application registered by": "Applicatie geregistreerd door", - "Who registered the application this version belongs to, copied from it. A supplier's application is public, and so are its versions.": "Wie de applicatie registreerde waar deze versie bij hoort, daarvan overgenomen. De applicatie van een leverancier is openbaar, en haar versies ook." + "Who registered the application this version belongs to, copied from it. A supplier's application is public, and so are its versions.": "Wie de applicatie registreerde waar deze versie bij hoort, daarvan overgenomen. De applicatie van een leverancier is openbaar, en haar versies ook.", + "The file could not be sent. Check your connection and try again.": "Het bestand kon niet worden verstuurd. Controleer uw verbinding en probeer het opnieuw.", + "The organisation register is not configured, so there are no organisations to choose from.": "Het organisatieregister is niet ingesteld, dus er zijn geen organisaties om uit te kiezen.", + "The organisations could not be loaded. Reload the page to try again.": "De organisaties konden niet worden geladen. Herlaad de pagina om het opnieuw te proberen.", + "Whether a service desk is connected could not be loaded. Reload the page to try again.": "Kon niet worden geladen of er een servicedesk is gekoppeld. Herlaad de pagina om het opnieuw te proberen." } } diff --git a/src/views/cmdb/CmdbOverview.vue b/src/views/cmdb/CmdbOverview.vue index 466be640..d705f76c 100644 --- a/src/views/cmdb/CmdbOverview.vue +++ b/src/views/cmdb/CmdbOverview.vue @@ -51,7 +51,15 @@

{{ t('stackiq', 'Service desk exchange') }}