From e7824552fe43bd3308c0890dd4e38db1db8b382d Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 12 Oct 2026 18:27:43 +0200 Subject: [PATCH 1/8] fix(zgw): a guard that cannot run its check refuses the request Decision 224. The case type delete check, drc-004 and drc-007 let a request through when their lookup failed. They now refuse with 503 check-not-completed, name the rule, and log the rule and the object. drc-003 gives the same answer instead of claiming a duplicate, and the controller's relation check before a document delete (drc-008a) follows. Two sites leave the catch-return-null list, ceiling 263 to 261. --- lib/Controller/DrcController.php | 95 +++++--- lib/Service/Zgw/DrcDocumentRelations.php | 202 ++++++++---------- lib/Service/Zgw/DrcObjectLinkRules.php | 14 +- lib/Service/Zgw/ZgwRuleLookups.php | 77 +++++-- lib/Service/Zgw/ZgwRuleResults.php | 77 ++++++- lib/Service/Zgw/ZtcPublishAndDeleteChecks.php | 75 ++++--- lib/Service/ZgwBusinessRulesService.php | 12 ++ lib/Service/ZgwDrcRulesService.php | 20 +- lib/Service/ZgwRulesBase.php | 2 +- lib/Service/ZgwZtcRulesService.php | 2 +- .../guards-refuse-when-unsure/proposal.md | 50 +++++ .../zgw-business-rules-compliance/spec.md | 64 ++++++ .../guards-refuse-when-unsure/tasks.md | 30 +++ .../catch-return-null.allowlist.json | 17 +- tests/Unit/Controller/DrcEioLifecycleTest.php | 34 ++- .../Service/Zgw/DrcDocumentRelationsTest.php | 75 +++++-- .../Service/Zgw/DrcObjectLinkRulesTest.php | 43 +++- tests/Unit/Service/Zgw/ZgwRuleLookupsTest.php | 31 +++ tests/Unit/Service/Zgw/ZgwRuleResultsTest.php | 64 +++++- .../Unit/Service/Zgw/ZtcConceptRulesTest.php | 2 +- .../Zgw/ZtcPublishAndDeleteChecksTest.php | 67 ++++-- ...gwBusinessRulesServiceDestroyGuardTest.php | 116 ++++++++++ tests/Unit/Service/ZgwDrcRulesServiceTest.php | 46 ++-- tests/Unit/Service/ZgwZtcRulesServiceTest.php | 33 ++- 24 files changed, 951 insertions(+), 297 deletions(-) create mode 100644 openspec/changes/guards-refuse-when-unsure/proposal.md create mode 100644 openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md create mode 100644 openspec/changes/guards-refuse-when-unsure/tasks.md create mode 100644 tests/Unit/Service/ZgwBusinessRulesServiceDestroyGuardTest.php diff --git a/lib/Controller/DrcController.php b/lib/Controller/DrcController.php index c68624e30d..8073a6fd86 100644 --- a/lib/Controller/DrcController.php +++ b/lib/Controller/DrcController.php @@ -30,6 +30,7 @@ namespace OCA\Dossiq\Controller; use OCA\Dossiq\Service\ZgwService; +use OCA\Dossiq\Service\Zgw\ZgwRuleResults; use OCA\Dossiq\Service\Zgw\ZgwSearchScope; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\AnonRateLimit; @@ -649,7 +650,8 @@ private function destroyGebruiksrecht(string $uuid): JSONResponse { /** * Delete an enkelvoudiginformatieobject. * - * Refused while ObjectInformatieObjecten point at it (drc-008a). After a successful delete its + * Refused while ObjectInformatieObjecten point at it (drc-008a), and refused when that could + * not be looked up. After a successful delete its * gebruiksrechten go (drc-008), and its stored files go when the document could be read * before the delete. * @@ -661,9 +663,14 @@ private function destroyEio(string $uuid): JSONResponse { $hasStoredFiles = $this->isReadableEio(uuid: $uuid); // Drc-008a (VNG): Block EIO deletion when OIO relations exist. - if ($this->zgwService->getObjectService() !== null - && empty($this->findOioRelationsForEio(eioUuid: $uuid)) === false - ) { + try { + $relations = $this->findOioRelationsForEio(eioUuid: $uuid); + } catch (\Throwable $e) { + // The search did not run, so nobody knows whether relations point here. Refuse. + return $this->relationCheckNotCompleted(uuid: $uuid, cause: $e->getMessage()); + } + + if (empty($relations) === false) { return new JSONResponse( [ 'detail' => $this->l10n->t('The document cannot be deleted: there are related ObjectInformatieObjecten.'), @@ -701,6 +708,26 @@ private function destroyEio(string $uuid): JSONResponse { return $response; }//end destroyEio() + /** + * The answer when the relation check of a document delete could not run (drc-008a). + * + * The same refusal the rules give, logged with the rule and the document. Nothing is deleted. + * + * @param string $uuid The document uuid. + * @param string $cause Why the check could not run. + * + * @return JSONResponse + */ + private function relationCheckNotCompleted(string $uuid, string $cause): JSONResponse { + $refusal = (new ZgwRuleResults(logger: $this->zgwService->getLogger())) + ->notVerified(rule: 'drc-008a', object: 'document ' . $uuid, cause: $cause); + + return new JSONResponse( + data: $this->zgwService->buildValidationError(ruleResult: $refusal), + statusCode: $refusal['status'] + ); + }//end relationCheckNotCompleted() + /** * Whether the document can be read, which is what decides whether its files are cleaned up. * @@ -1177,6 +1204,8 @@ public function audittrailShow(string $resource, string $uuid, string $auditUuid * @param string $eioUuid The EIO UUID * * @return array List of related object IDs linked to this EIO + * + * @throws \Throwable When a schema cannot be searched. */ private function findOioRelationsForEio(string $eioUuid): array { $objectService = $this->zgwService->getObjectService(); @@ -1237,7 +1266,9 @@ private function findOioRelationsForEio(string $eioUuid): array { * @param string $register The register ID * @param string $schema The schema ID * - * @return array List of related object IDs + * @return array List of related object IDs. Empty means the search ran and found none. + * + * @throws \Throwable When the schema cannot be searched. */ private function searchRelationsInSchema( object $objectService, @@ -1245,36 +1276,30 @@ private function searchRelationsInSchema( string $register, string $schema, ): array { - try { - // Try exact UUID match (OIO may store just the UUID). - $query = $objectService->buildSearchQuery( - requestParams: ['document' => $eioUuid, '_limit' => 1], - register: $register, - schema: $schema - ); - $result = $objectService->searchObjectsPaginated(query: $query); - $ids = $this->extractIdsFromResults(result: $result); - if (empty($ids) === false) { - return $ids; - } - - // Fallback: full-text search by UUID (document field stores - // the full URL, and field-specific LIKE is not supported). - $query = $objectService->buildSearchQuery( - requestParams: ['_search' => $eioUuid, '_limit' => 1], - register: $register, - schema: $schema - ); - $result = $objectService->searchObjectsPaginated(query: $query); - $ids = $this->extractIdsFromResults(result: $result); - if (empty($ids) === false) { - return $ids; - } - } catch (\Throwable $e) { - $this->zgwService->getLogger()->warning( - 'drc-008a: Relation search failed for schema ' . $schema . ': ' . $e->getMessage() - ); - }//end try + // Try exact UUID match (OIO may store just the UUID). + $query = $objectService->buildSearchQuery( + requestParams: ['document' => $eioUuid, '_limit' => 1], + register: $register, + schema: $schema + ); + $result = $objectService->searchObjectsPaginated(query: $query); + $ids = $this->extractIdsFromResults(result: $result); + if (empty($ids) === false) { + return $ids; + } + + // Fallback: full-text search by UUID (document field stores + // the full URL, and field-specific LIKE is not supported). + $query = $objectService->buildSearchQuery( + requestParams: ['_search' => $eioUuid, '_limit' => 1], + register: $register, + schema: $schema + ); + $result = $objectService->searchObjectsPaginated(query: $query); + $ids = $this->extractIdsFromResults(result: $result); + if (empty($ids) === false) { + return $ids; + } return []; }//end searchRelationsInSchema() diff --git a/lib/Service/Zgw/DrcDocumentRelations.php b/lib/Service/Zgw/DrcDocumentRelations.php index 1691dea983..78a0d99ce1 100644 --- a/lib/Service/Zgw/DrcDocumentRelations.php +++ b/lib/Service/Zgw/DrcDocumentRelations.php @@ -31,7 +31,6 @@ use OCA\Dossiq\Service\FieldValidator; use OCA\Dossiq\Service\SettingsService; -use Psr\Log\LoggerInterface; /** * Reads the stored relations of a document. @@ -46,14 +45,12 @@ class DrcDocumentRelations { * @param ZgwRuleResults $results Builds the answers of a rule. * @param SettingsService $settingsService The settings service. * @param FieldValidator $fieldValidator The field validator. - * @param LoggerInterface $logger The logger. */ public function __construct( private readonly ZgwRuleLookups $lookups, private readonly ZgwRuleResults $results, private readonly SettingsService $settingsService, private readonly FieldValidator $fieldValidator, - private readonly LoggerInterface $logger, ) { }//end __construct() @@ -63,12 +60,17 @@ public function __construct( * OIO objects store the full informatieobject URL in the 'document' field. * This method searches by UUID first, then by partial URL match if needed. * + * An empty list means both searches ran and found none. A search that cannot + * run throws, so the caller refuses the delete instead of reading "no relations". + * * @param string $register The register ID * @param string $schema The OIO schema ID * @param string $docUuid The document UUID * * @return array Array of matching OIO UUIDs * + * @throws \Throwable When the store cannot be searched. + * * @spec openspec/specs/zgw-business-rules-compliance/spec.md */ public function findOioRelationsForDocument( @@ -77,7 +79,7 @@ public function findOioRelationsForDocument( string $docUuid, ): array { // First try by exact UUID (in case stored as UUID). - $results = $this->lookups->findAllObjectsByField( + $results = $this->lookups->idsByField( register: $register, schema: $schema, field: 'document', @@ -88,56 +90,50 @@ public function findOioRelationsForDocument( } // Also search by partial URL match (OIO may store full URL). - try { - $query = $this->lookups->objectService()->buildSearchQuery( - requestParams: ['document' => '%' . $docUuid . '%', '_limit' => 100], - register: $register, - schema: $schema - ); - $result = $this->lookups->objectService()->searchObjectsPaginated(query: $query); - - $ids = []; - foreach (($result['results'] ?? []) as $obj) { - $data = $obj; - if (is_array($obj) === false) { - $data = $obj->jsonSerialize(); - } - - $id = $data['id'] ?? ($data['@self']['id'] ?? null); - if ($id !== null) { - $ids[] = $id; - } - } - - return $ids; - } catch (\Throwable $e) { - $this->logger->warning( - 'Drc-007: OIO relation search with partial match failed: ' . $e->getMessage() - ); - return []; - }//end try + return $this->lookups->idsByField( + register: $register, + schema: $schema, + field: 'document', + value: '%' . $docUuid . '%' + ); }//end findOioRelationsForDocument() /** * Validate OIO uniqueness: object + informatieobject + objectType (drc-003 VNG). * + * A search that cannot run refuses with the "could not be checked" answer. + * It does not answer "duplicate": that would tell the caller a relation + * exists that nobody found. + * * @param string $ioUrl The informatieobject URL * @param string $objectUrl The object URL (zaak or besluit) * @param string $objectType The object type (zaak or besluit) * - * @return array|null Validation error if duplicate found, null if unique + * @return array|null The refusal, or null when the relation is new * * @spec openspec/specs/zgw-business-rules-compliance/spec.md */ public function checkOioUniqueness(string $ioUrl, string $objectUrl, string $objectType): ?array { $register = $this->lookups->register(); - $oioSchema = $this->settingsService->getConfigValue(key: 'document_link_schema'); - if ($register === '') { return null; } - $duplicateError = $this->results->error( + try { + $duplicate = $this->relationExists(register: $register, ioUrl: $ioUrl, objectUrl: $objectUrl, objectType: $objectType); + } catch (\Throwable $e) { + return $this->results->notVerified( + rule: 'drc-003', + object: 'document ' . $ioUrl . ' on ' . $objectUrl, + cause: $e->getMessage() + ); + } + + if ($duplicate === false) { + return null; + } + + return $this->results->error( status: 400, detail: 'De combinatie informatieobject + object + objectType bestaat al.', invalidParams: [ @@ -148,59 +144,55 @@ public function checkOioUniqueness(string $ioUrl, string $objectUrl, string $obj ), ] ); + }//end checkOioUniqueness() + + /** + * Whether the relation is stored already, as an OIO or as its ZIO or BIO. + * + * Drc-003a/b: a relationship created via ZRC (zaakinformatieobjecten) or BRC + * (besluitinformatieobjecten) counts as a duplicate for OIO creation. + * + * @param string $register The register ID + * @param string $ioUrl The informatieobject URL + * @param string $objectUrl The object URL (zaak or besluit) + * @param string $objectType The object type (zaak or besluit) + * + * @return bool True when a stored relation was found + * + * @throws \Throwable When the store cannot be searched. + */ + private function relationExists(string $register, string $ioUrl, string $objectUrl, string $objectType): bool { + $crossRegister = ['case' => ['case_document_schema', 'case'], 'decision' => ['decision_document_schema', 'decision']]; + $places = [['document_link_schema', 'object']]; + if (isset($crossRegister[$objectType]) === true) { + $places[] = $crossRegister[$objectType]; + } $ioUuid = $this->fieldValidator->extractUuid(url: $ioUrl); $objectUuid = $this->fieldValidator->extractUuid(url: $objectUrl); - // Check OIO schema for existing duplicates. - if ($oioSchema !== '') { + foreach ($places as [$schemaKey, $objectField]) { + $schema = $this->settingsService->getConfigValue(key: $schemaKey); + if ($schema === '') { + continue; + } + $found = $this->searchDuplicateRelation( register: $register, - schema: $oioSchema, + schema: $schema, ioUrl: $ioUrl, objectUrl: $objectUrl, ioUuid: $ioUuid, objectUuid: $objectUuid, - objectField: 'object' + objectField: $objectField ); if ($found === true) { - return $duplicateError; - } - } - - // Drc-003a/b: Also check ZIO/BIO schemas — a relationship created via - // ZRC (zaakinformatieobjecten) or BRC (besluitinformatieobjecten) counts - // as a duplicate for OIO creation. - $crossSchemaKey = ''; - $crossField = ''; - if ($objectType === 'case') { - $crossSchemaKey = 'case_document_schema'; - $crossField = 'case'; - } elseif ($objectType === 'decision') { - $crossSchemaKey = 'decision_document_schema'; - $crossField = 'decision'; - } - - if ($crossSchemaKey !== '') { - $crossSchema = $this->settingsService->getConfigValue(key: $crossSchemaKey); - if ($crossSchema !== '') { - $found = $this->searchDuplicateRelation( - register: $register, - schema: $crossSchema, - ioUrl: $ioUrl, - objectUrl: $objectUrl, - ioUuid: $ioUuid, - objectUuid: $objectUuid, - objectField: $crossField - ); - if ($found === true) { - return $duplicateError; - } + return true; } } - return null; - }//end checkOioUniqueness() + return false; + }//end relationExists() /** * Search for an existing relation in a schema by document+object combination. @@ -214,6 +206,8 @@ public function checkOioUniqueness(string $ioUrl, string $objectUrl, string $obj * @param string $objectField The field name for the object reference * * @return bool True if a duplicate was found + * + * @throws \Throwable When the store cannot be searched. */ private function searchDuplicateRelation( string $register, @@ -224,10 +218,21 @@ private function searchDuplicateRelation( ?string $objectUuid, string $objectField, ): bool { - try { - // Search by full URL. + // Search by full URL. + $query = $this->lookups->objectService()->buildSearchQuery( + requestParams: ['document' => $ioUrl, $objectField => $objectUrl, '_limit' => 1], + register: $register, + schema: $schema + ); + $result = $this->lookups->objectService()->searchObjectsPaginated(query: $query); + if (($result['total'] ?? count($result['results'] ?? [])) > 0) { + return true; + } + + // Fallback: search by UUID (stored data may use UUID instead of URL). + if ($ioUuid !== null && $objectUuid !== null) { $query = $this->lookups->objectService()->buildSearchQuery( - requestParams: ['document' => $ioUrl, $objectField => $objectUrl, '_limit' => 1], + requestParams: ['document' => $ioUuid, $objectField => $objectUuid, '_limit' => 1], register: $register, schema: $schema ); @@ -236,39 +241,18 @@ private function searchDuplicateRelation( return true; } - // Fallback: search by UUID (stored data may use UUID instead of URL). - if ($ioUuid !== null && $objectUuid !== null) { - $query = $this->lookups->objectService()->buildSearchQuery( - requestParams: ['document' => $ioUuid, $objectField => $objectUuid, '_limit' => 1], - register: $register, - schema: $schema - ); - $result = $this->lookups->objectService()->searchObjectsPaginated(query: $query); - if (($result['total'] ?? count($result['results'] ?? [])) > 0) { - return true; - } - - // Full-text search using both UUIDs (field-specific LIKE - // with % is not supported by MagicMapper). - $query = $this->lookups->objectService()->buildSearchQuery( - requestParams: ['_search' => $ioUuid . ' ' . $objectUuid, '_limit' => 1], - register: $register, - schema: $schema - ); - $result = $this->lookups->objectService()->searchObjectsPaginated(query: $query); - if (($result['total'] ?? count($result['results'] ?? [])) > 0) { - return true; - } - }//end if - } catch (\Throwable $e) { - // Fail-closed: treat an indeterminate check as a duplicate to prevent - // inadvertent duplicates when the storage backend is unavailable. - $this->logger->warning( - 'Drc-003: Uniqueness check failed for schema {schema}: {message}', - ['schema' => $schema, 'message' => $e->getMessage()] + // Full-text search using both UUIDs (field-specific LIKE + // with % is not supported by MagicMapper). + $query = $this->lookups->objectService()->buildSearchQuery( + requestParams: ['_search' => $ioUuid . ' ' . $objectUuid, '_limit' => 1], + register: $register, + schema: $schema ); - return true; - }//end try + $result = $this->lookups->objectService()->searchObjectsPaginated(query: $query); + if (($result['total'] ?? count($result['results'] ?? [])) > 0) { + return true; + } + }//end if return false; }//end searchDuplicateRelation() diff --git a/lib/Service/Zgw/DrcObjectLinkRules.php b/lib/Service/Zgw/DrcObjectLinkRules.php index cb11cc96c6..ae0372b6e0 100644 --- a/lib/Service/Zgw/DrcObjectLinkRules.php +++ b/lib/Service/Zgw/DrcObjectLinkRules.php @@ -31,7 +31,6 @@ use OCA\Dossiq\Service\FieldValidator; use OCA\Dossiq\Service\SettingsService; -use Psr\Log\LoggerInterface; /** * Create rules of an objectinformatieobject. @@ -46,7 +45,6 @@ class DrcObjectLinkRules { * @param ZgwRuleResults $results Builds the answers of a rule. * @param SettingsService $settingsService The settings service. * @param FieldValidator $fieldValidator The field validator. - * @param LoggerInterface $logger The logger. * @param DrcDocumentRelations $relations The stored relations of a document. */ public function __construct( @@ -54,7 +52,6 @@ public function __construct( private readonly ZgwRuleResults $results, private readonly SettingsService $settingsService, private readonly FieldValidator $fieldValidator, - private readonly LoggerInterface $logger, private readonly DrcDocumentRelations $relations, ) { }//end __construct() @@ -99,7 +96,7 @@ public function rulesObjectinformatieobjectenCreate(array $body): array { * @param mixed $objectUrl The object URL * @param mixed $objectType The objectType * - * @return array|null The refusal, or null when acceptable or nothing can be looked up + * @return array|null The refusal, or null when acceptable or when there is nothing to look up */ private function checkOioRelations(mixed $ioUrl, mixed $objectUrl, mixed $objectType): ?array { if ($ioUrl === '' || $objectUrl === '' || $this->lookups->objectService() === null) { @@ -189,7 +186,7 @@ private function validateObjectUrl(string $objectUrl, string $objectType): ?arra * @param string $objectUrl The object URL (zaak or besluit) * @param string $objectType The object type (zaak or besluit) * - * @return array|null Validation error if relation doesn't exist, null if valid + * @return array|null The refusal when the relation does not exist or could not be looked up, null if valid */ private function validateOioCrossRegister(string $ioUrl, string $objectUrl, string $objectType): ?array { $ioUuid = $this->fieldValidator->extractUuid(url: $ioUrl) ?? ''; @@ -251,8 +248,11 @@ private function validateOioCrossRegister(string $ioUrl, string $objectUrl, stri ); } } catch (\Throwable $e) { - $this->logger->warning( - 'Drc-004: Cross-register validation failed: ' . $e->getMessage() + // The lookup did not run, so nobody knows whether the counterpart exists. Refuse. + return $this->results->notVerified( + rule: 'drc-004', + object: $objectType . ' ' . $objectUuid . ' with document ' . $ioUuid, + cause: $e->getMessage() ); }//end try diff --git a/lib/Service/Zgw/ZgwRuleLookups.php b/lib/Service/Zgw/ZgwRuleLookups.php index af9e6b1285..119cfb00b5 100644 --- a/lib/Service/Zgw/ZgwRuleLookups.php +++ b/lib/Service/Zgw/ZgwRuleLookups.php @@ -30,6 +30,7 @@ use OCA\Dossiq\Service\SettingsService; use Psr\Log\LoggerInterface; +use RuntimeException; /** * Store lookups of the ZGW business rules, in the request context. @@ -150,6 +151,9 @@ public function findObjectByField( /** * Find all objects matching a field value. * + * A search that fails is logged and answers an empty list. A rule that must + * tell "none" from "could not look" reads {@see self::idsByField()} instead. + * * @param string $register The register to search in * @param string $schema The schema to search in * @param string $field The field to match on @@ -166,36 +170,65 @@ public function findAllObjectsByField( string $value, ): array { try { - $query = $this->objectService->buildSearchQuery( - requestParams: [$field => $value, '_limit' => 100], - register: $register, - schema: $schema - ); - $result = $this->objectService->searchObjectsPaginated(query: $query); - - $ids = []; - foreach (($result['results'] ?? []) as $obj) { - $data = $obj; - if (is_array($obj) === false) { - $data = $obj->jsonSerialize(); - } - - $id = $data['id'] ?? ($data['@self']['id'] ?? null); - if ($id !== null) { - $ids[] = $id; - } - } - - return $ids; + return $this->idsByField(register: $register, schema: $schema, field: $field, value: $value); } catch (\Throwable $e) { $this->logger->warning( 'Reference resolution failed: ' . $e->getMessage(), ['field' => $field, 'value' => $value] ); return []; - }//end try + } }//end findAllObjectsByField() + /** + * The ids of the objects that carry a field value. A search that cannot run throws. + * + * An empty list means the search ran and found none. + * + * @param string $register The register to search in + * @param string $schema The schema to search in + * @param string $field The field to match on + * @param string $value The field value to search for + * + * @return array The ids of the matching objects + * + * @throws \Throwable When the store cannot be searched. + * + * @spec openspec/specs/zgw-business-rules-compliance/spec.md + */ + public function idsByField( + string $register, + string $schema, + string $field, + string $value, + ): array { + if ($this->objectService === null) { + throw new RuntimeException('No object service in the request context'); + } + + $query = $this->objectService->buildSearchQuery( + requestParams: [$field => $value, '_limit' => 100], + register: $register, + schema: $schema + ); + $result = $this->objectService->searchObjectsPaginated(query: $query); + + $ids = []; + foreach (($result['results'] ?? []) as $obj) { + $data = $obj; + if (is_array($obj) === false) { + $data = $obj->jsonSerialize(); + } + + $id = $data['id'] ?? ($data['@self']['id'] ?? null); + if ($id !== null) { + $ids[] = $id; + } + } + + return $ids; + }//end idsByField() + /** * Look up an object in OpenRegister by UUID and schema key. * diff --git a/lib/Service/Zgw/ZgwRuleResults.php b/lib/Service/Zgw/ZgwRuleResults.php index 5c67b99999..43978799ac 100644 --- a/lib/Service/Zgw/ZgwRuleResults.php +++ b/lib/Service/Zgw/ZgwRuleResults.php @@ -4,8 +4,12 @@ * The answers a ZGW business rule gives * * A rule passes with the body it may have enriched, or refuses with a status, - * a detail and the fields it refuses. Stateless: every rules class and every - * collaborator builds the same shapes here. + * a detail and the fields it refuses. Every rules class and every collaborator + * builds the same shapes here. + * + * A rule that could not run its check refuses too, with its own answer: the + * request is not at fault, so it gets no field errors, and the log names the + * rule and the object. * * @category Service * @package OCA\Dossiq\Service\Zgw @@ -28,12 +32,79 @@ namespace OCA\Dossiq\Service\Zgw; +use Psr\Log\LoggerInterface; + /** * Builds the valid and the refused result of a rule. * * @spec openspec/specs/zgw-business-rules-compliance/spec.md */ class ZgwRuleResults { + /** + * The status of a refusal because the check could not run. + * + * The same status the ZGW endpoints answer when OpenRegister is absent. + */ + public const NOT_VERIFIED_STATUS = 503; + + /** + * The code of a refusal because the check could not run. + */ + public const NOT_VERIFIED_CODE = 'check-not-completed'; + + /** + * Constructor. + * + * @param LoggerInterface $logger The logger. + */ + public function __construct( + private readonly LoggerInterface $logger, + ) { + }//end __construct() + + /** + * Refuse because a rule could not run its check, and log which rule and which object. + * + * A lookup that ran and found nothing is an answer. A lookup that could not + * run is not, so the rule refuses instead of passing. The refusal carries no + * field errors: nothing in the request is wrong. + * + * @param string $rule The rule, as the standard names it (drc-004). + * @param string $object What the rule was checking. + * @param string $cause Why the check could not run. + * + * @return array{valid: bool, status: int, detail: string, invalidParams: array, enrichedBody: array} With `code` set. + * + * @spec openspec/specs/zgw-business-rules-compliance/spec.md + */ + public function notVerified(string $rule, string $object, string $cause): array { + $this->logNotVerified(rule: $rule, object: $object, cause: $cause); + + return $this->error( + status: self::NOT_VERIFIED_STATUS, + detail: "De controle van regel {$rule} kon niet worden uitgevoerd. Er is niets gewijzigd. Probeer het later opnieuw.", + code: self::NOT_VERIFIED_CODE + ); + }//end notVerified() + + /** + * Log that a rule could not run its check and that the request is refused for it. + * + * @param string $rule The rule. + * @param string $object What the rule was checking. + * @param string $cause Why the check could not run. + * + * @return void + * + * @spec openspec/specs/zgw-business-rules-compliance/spec.md + */ + public function logNotVerified(string $rule, string $object, string $cause): void { + $this->logger->error( + 'ZGW rule {rule} could not be checked for {object}, so the request is refused: {cause}', + ['rule' => $rule, 'object' => $object, 'cause' => $cause] + ); + }//end logNotVerified() + /** * Build a successful validation result (pass-through). * @@ -55,7 +126,7 @@ public function isValid(array $body): array { /** * Build a validation error result. * - * @param int $status HTTP status code (400 or 403) + * @param int $status HTTP status code (400, 403, or 503 when the check could not run) * @param string $detail Error detail message * @param array $invalidParams Invalid parameter entries * @param string $code Optional error code diff --git a/lib/Service/Zgw/ZtcPublishAndDeleteChecks.php b/lib/Service/Zgw/ZtcPublishAndDeleteChecks.php index 5d89273122..d946fe8b56 100644 --- a/lib/Service/Zgw/ZtcPublishAndDeleteChecks.php +++ b/lib/Service/Zgw/ZtcPublishAndDeleteChecks.php @@ -5,7 +5,8 @@ * * Publishing needs a status type, a final one among them, and a valid-from date. * Deleting is blocked while active cases use the type and asks for confirmation - * when only closed cases do. + * when only closed cases do. A delete check that cannot read the cases blocks + * too: "could not look" is not "no cases". * * @category Service * @package OCA\Dossiq\Service\Zgw @@ -31,6 +32,7 @@ use OCA\Dossiq\Service\SettingsService; use OCA\Dossiq\Service\Support\SearchesObjects; +use RuntimeException; /** * Publish and delete checks of a case type. @@ -40,14 +42,21 @@ class ZtcPublishAndDeleteChecks { use SearchesObjects; + /** + * The name the delete check goes by in the log. + */ + private const DELETE_CHECK = 'ztc case type delete check (REQ-CT-01d)'; + /** * Constructor. * * @param ZgwRuleLookups $lookups Store lookups in the request context. + * @param ZgwRuleResults $results Logs a check that could not run. * @param SettingsService $settingsService The settings service. */ public function __construct( private readonly ZgwRuleLookups $lookups, + private readonly ZgwRuleResults $results, private readonly SettingsService $settingsService, ) { }//end __construct() @@ -169,8 +178,11 @@ private function loadCaseTypeRow(object $objectService, string $register, string * ['blocked' => bool, 'requiresConfirmation' => bool, 'message' => string] * * - blocked=true → 409 Conflict; active cases exist. + * - blocked=true with unverified=true → the cases could not be read, so nobody + * knows whether the type is in use. The caller refuses with the + * "could not be checked" status, not with a conflict. * - requiresConfirmation=true → 200 OK with confirmation prompt (closed-only cases). - * - otherwise → safe to delete. + * - otherwise → the check ran and found no cases: safe to delete. * * @param string $register Register slug. * @param string $caseTypeId Case type id. @@ -181,32 +193,39 @@ private function loadCaseTypeRow(object $objectService, string $register, string */ public function validateDeletion(string $register, string $caseTypeId): array { $default = ['blocked' => false, 'requiresConfirmation' => false, 'message' => '']; - if ($this->lookups->objectService() === null || $caseTypeId === '') { + if ($caseTypeId === '') { return $default; } - $caseSchema = (string)$this->settingsService->getConfigValue(key: 'case_schema'); - try { + $objectService = $this->lookups->objectService(); + if ($objectService === null) { + throw new RuntimeException('No object service in the request context'); + } + $cases = $this->searchObjectsAsArrays( - objectService: $this->lookups->objectService(), + objectService: $objectService, register: $register, - schema: $caseSchema, + schema: (string)$this->settingsService->getConfigValue(key: 'case_schema'), filters: ['caseType' => $caseTypeId], ); - } catch (\Throwable $e) { - return $default; - } + if (count($cases) === 0) { + return $default; + } - if (count($cases) === 0) { - return $default; - } + $finalSlugs = $this->loadFinalStatusSlugs(objectService: $objectService, register: $register, caseTypeId: $caseTypeId); + } catch (\Throwable $e) { + // The cases or their final statuses could not be read. Refuse: the type may be in use. + $this->results->logNotVerified(rule: self::DELETE_CHECK, object: 'case type ' . $caseTypeId, cause: $e->getMessage()); - $finalSlugs = $this->loadFinalStatusSlugs( - objectService: $this->lookups->objectService(), - register: $register, - caseTypeId: $caseTypeId, - ); + return [ + 'blocked' => true, + 'requiresConfirmation' => false, + 'unverified' => true, + 'message' => 'Cannot delete case type: the check for cases that use this type could not be completed. ' + . 'Nothing was deleted. Try again later.', + ]; + }//end try $tally = $this->tallyCaseClosure(cases: $cases, finalSlugs: $finalSlugs); $activeCount = $tally['active']; @@ -229,26 +248,24 @@ public function validateDeletion(string $register, string $caseTypeId): array { }//end validateDeletion() /** - * Load the ids of the final statusTypes of a case type, or an empty list when unreadable. + * Load the ids of the final statusTypes of a case type. An unreadable list throws. * * @param object $objectService The OpenRegister object service. * @param string $register Register slug. * @param string $caseTypeId Case type id. * * @return array The final statusType ids. + * + * @throws \Throwable When the status types cannot be read. */ private function loadFinalStatusSlugs(object $objectService, string $register, string $caseTypeId): array { $statusSchema = (string)$this->settingsService->getConfigValue(key: 'status_type_schema'); - try { - $finalStatusTypes = $this->searchObjectsAsArrays( - objectService: $objectService, - register: $register, - schema: $statusSchema, - filters: ['caseType' => $caseTypeId, 'isFinal' => true], - ); - } catch (\Throwable $e) { - return []; - } + $finalStatusTypes = $this->searchObjectsAsArrays( + objectService: $objectService, + register: $register, + schema: $statusSchema, + filters: ['caseType' => $caseTypeId, 'isFinal' => true], + ); $finalSlugs = []; foreach ($finalStatusTypes as $row) { diff --git a/lib/Service/ZgwBusinessRulesService.php b/lib/Service/ZgwBusinessRulesService.php index f15d18550d..ce814f8fd9 100644 --- a/lib/Service/ZgwBusinessRulesService.php +++ b/lib/Service/ZgwBusinessRulesService.php @@ -31,6 +31,7 @@ namespace OCA\Dossiq\Service; +use OCA\Dossiq\Service\Zgw\ZgwRuleResults; use OCA\Dossiq\Service\Zgw\ZgwRulesDispatcher; /** @@ -282,6 +283,17 @@ private function guardCaseTypeDestroy( } $delGuard = $this->ztcRules->validateDeletion($register, $caseTypeId); + if (($delGuard['unverified'] ?? false) === true) { + // The check could not read the cases. Nothing says the request is wrong, so no conflict. + return [ + 'valid' => false, + 'status' => ZgwRuleResults::NOT_VERIFIED_STATUS, + 'detail' => (string)$delGuard['message'], + 'code' => 'destroy_check_not_completed', + 'enrichedBody' => $body, + ]; + } + if ($delGuard['blocked'] === true) { return [ 'valid' => false, diff --git a/lib/Service/ZgwDrcRulesService.php b/lib/Service/ZgwDrcRulesService.php index 6a727b95bb..7e1fc6f686 100644 --- a/lib/Service/ZgwDrcRulesService.php +++ b/lib/Service/ZgwDrcRulesService.php @@ -107,15 +107,13 @@ public function __construct(LoggerInterface $logger, SettingsService $settingsSe lookups: $this->lookups, results: $this->results, settingsService: $settingsService, - fieldValidator: $fieldValidator, - logger: $logger + fieldValidator: $fieldValidator ); $this->objectLinks = new DrcObjectLinkRules( lookups: $this->lookups, results: $this->results, settingsService: $settingsService, fieldValidator: $fieldValidator, - logger: $logger, relations: $this->relations ); }//end __construct() @@ -329,11 +327,17 @@ public function rulesEnkelvoudiginformatieobjectenDestroy( if ($register !== '' && $oioSchema !== '') { // OIO stores the full informatieobject URL in the 'document' field, // so search with both UUID and partial match to find any OIO relations. - $relatedIds = $this->relations->findOioRelationsForDocument( - register: $register, - schema: $oioSchema, - docUuid: $existingId - ); + try { + $relatedIds = $this->relations->findOioRelationsForDocument( + register: $register, + schema: $oioSchema, + docUuid: $existingId + ); + } catch (\Throwable $e) { + // The search did not run, so nobody knows whether relations point here. Refuse. + return $this->results->notVerified(rule: 'drc-007', object: 'document ' . $existingId, cause: $e->getMessage()); + } + if (empty($relatedIds) === false) { return $this->error( status: 400, diff --git a/lib/Service/ZgwRulesBase.php b/lib/Service/ZgwRulesBase.php index 38effd6ef9..59787d08d0 100644 --- a/lib/Service/ZgwRulesBase.php +++ b/lib/Service/ZgwRulesBase.php @@ -111,7 +111,7 @@ public function __construct( protected readonly SettingsService $settingsService, protected readonly FieldValidator $fieldValidator, ) { - $this->results = new ZgwRuleResults(); + $this->results = new ZgwRuleResults(logger: $logger); $this->lookups = new ZgwRuleLookups(logger: $logger, settingsService: $settingsService); }//end __construct() diff --git a/lib/Service/ZgwZtcRulesService.php b/lib/Service/ZgwZtcRulesService.php index 862ecda58c..599b77a5ad 100644 --- a/lib/Service/ZgwZtcRulesService.php +++ b/lib/Service/ZgwZtcRulesService.php @@ -117,7 +117,7 @@ public function __construct(LoggerInterface $logger, SettingsService $settingsSe parent::__construct(logger: $logger, settingsService: $settingsService, fieldValidator: $fieldValidator); $this->concept = new ZtcConceptRules(results: $this->results); $this->references = new ZtcTypeReferences(lookups: $this->lookups, settingsService: $settingsService, fieldValidator: $fieldValidator); - $this->lifecycle = new ZtcPublishAndDeleteChecks(lookups: $this->lookups, settingsService: $settingsService); + $this->lifecycle = new ZtcPublishAndDeleteChecks(lookups: $this->lookups, results: $this->results, settingsService: $settingsService); }//end __construct() /** diff --git a/openspec/changes/guards-refuse-when-unsure/proposal.md b/openspec/changes/guards-refuse-when-unsure/proposal.md new file mode 100644 index 0000000000..2d38ee75c5 --- /dev/null +++ b/openspec/changes/guards-refuse-when-unsure/proposal.md @@ -0,0 +1,50 @@ +--- +kind: code +depends_on: [] +--- + +# Proposal: guards-refuse-when-unsure + +## Summary + +A ZGW guard that cannot run its check refuses the request. It no longer passes, and it no longer answers something it did not find. + +- Decision: Ruben, 2026-10-12, decision 224. "Guards refuse when unsure: the case type delete check and document rules drc-004 and drc-007 refuse with a clear error when their check cannot run, like drc-003." + +## Why + +Three guards read the store to decide. When the read failed they let the request through: + +- the case type delete check deleted a case type whose cases it could not read; +- drc-004 stored a link between a document and a case or decision without knowing the counterpart exists; +- drc-007 deleted a document without knowing whether relations still point at it. + +A fourth, drc-003, did refuse, but said "this combination already exists" about a relation nobody had found. A client that reads that answer stops retrying a request that was never wrong. + +"The lookup ran and found nothing" and "the lookup could not run" are two answers. Only the first is a pass. + +## What changes + +1. One refusal for a check that could not run: HTTP 503, code `check-not-completed`, a detail that names the rule, and no `invalidParams` (REQ-ZBR-020). +2. drc-003, drc-004, drc-007 and the controller's own relation check before a document delete (drc-008a) answer it when their search fails. +3. The case type delete check blocks the delete as unverified when it cannot read the cases or the final statuses. The ZGW delete answers 503 with code `destroy_check_not_completed` (REQ-ZBR-021). +4. Every such refusal is logged at error with the rule and the object. + +## Why 503 + +The ZGW error model has two shapes. A `ValidatieFout` (400, with `invalidParams`) says the request is wrong and names the field to fix. A `Fout` covers everything else. A check that did not run says nothing about the request, so it is a `Fout`, and a 4xx would tell the client to change a body that was fine. + +Of the 5xx statuses the ZGW specifications declare 500 on every operation. These endpoints already answer 503 with a `detail` when OpenRegister is absent (`ZgwService::unavailableResponse`). A store that cannot be read is the same situation, so the guards answer the same status. It also tells a client the one useful thing: the request can be sent again unchanged. + +The status is one constant, `ZgwRuleResults::NOT_VERIFIED_STATUS`. + +## Out of scope + +- A setting that is not configured. A rule whose schema setting is empty still skips its lookup, as drc-003 does. +- The cross-register writes of the Besluiten API, which stay best effort, and the 404 a failed ZGW delete answers. Both are in QUESTIONS.md (Q-dossiq-L11-2). +- The other lookups in `ZgwRuleLookups` that answer "none" when a read fails (Q-dossiq-L11-3). + +## Release notes + +- A ZGW request is refused with 503 `check-not-completed` when dossiq could not check a business rule. Nothing is stored or deleted. Send the request again. +- Deleting a case type answers 503 `destroy_check_not_completed` when dossiq could not read the cases of that type. diff --git a/openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md b/openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md new file mode 100644 index 0000000000..83a0bb8ef9 --- /dev/null +++ b/openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md @@ -0,0 +1,64 @@ +# zgw-business-rules-compliance + +## ADDED Requirements + +### Requirement: A rule that cannot run its check refuses the request (REQ-ZBR-020) + +A ZGW business rule that reads the store to decide SHALL tell "the lookup ran and found nothing" from "the lookup could not run". The first is an answer and the rule decides on it. The second is not, and the rule SHALL refuse the request. + +The refusal SHALL be HTTP 503 with a `detail` that names the rule and says the check could not be completed, and the `code` `check-not-completed`. It SHALL carry no `invalidParams`: nothing in the request is wrong. Nothing SHALL be stored or deleted. + +Every such refusal SHALL be logged at error with the rule and the object it was checking. + +This holds for drc-003 (a relation is new), drc-004 (the ZaakInformatieObject or BesluitInformatieObject exists), drc-007 (no ObjectInformatieObject points at the document) and the relation check before a document delete (drc-008a). + +A rule SHALL NOT answer a finding it did not make. A duplicate search that failed is not a duplicate. + +#### Scenario: The counterpart lookup of drc-004 fails + +- **GIVEN** an objectinformatieobject for a case and a document +- **WHEN** the search for the ZaakInformatieObject throws +- **THEN** the create is refused with 503 and code `check-not-completed` +- **AND** the detail names drc-004 +- **AND** the log holds the rule, the case and the document + +#### Scenario: The counterpart lookup of drc-004 finds nothing + +- **GIVEN** the same objectinformatieobject +- **WHEN** the search runs and finds no ZaakInformatieObject +- **THEN** the create is refused with 400 and code `inconsistent-relation` + +#### Scenario: The relation search of a document delete fails + +- **GIVEN** a document +- **WHEN** a search for the relations that point at it throws +- **THEN** the delete is refused with 503 and code `check-not-completed` +- **AND** the document, its gebruiksrechten and its files stay + +#### Scenario: The duplicate search of drc-003 fails + +- **GIVEN** an objectinformatieobject nobody stored before +- **WHEN** the duplicate search throws +- **THEN** the create is refused with 503 and code `check-not-completed` +- **AND** the answer does not say the combination exists + +### Requirement: A case type is not deleted when its cases cannot be read (REQ-ZBR-021) + +The delete check of a case type SHALL block the delete when it cannot read the cases that use the type, or the final statuses that say which of them are closed. The ZGW delete SHALL answer 503 with the code `destroy_check_not_completed` and a detail that says the check could not be completed. A confirmation flag SHALL NOT override it. + +A check that ran and found no cases SHALL let the delete through, as before. Active cases stay a 409 `destroy_blocked_active_cases`, and closed cases stay a 409 `destroy_requires_confirmation` until confirmed. + +The refusal SHALL be logged at error with the check and the case type. + +#### Scenario: The cases cannot be read + +- **GIVEN** a case type +- **WHEN** the search for its cases throws +- **THEN** the delete is refused with 503 and code `destroy_check_not_completed` +- **AND** the case type stays + +#### Scenario: Nobody uses the case type + +- **GIVEN** a case type +- **WHEN** the search runs and finds no cases +- **THEN** the delete goes through diff --git a/openspec/changes/guards-refuse-when-unsure/tasks.md b/openspec/changes/guards-refuse-when-unsure/tasks.md new file mode 100644 index 0000000000..5154156d89 --- /dev/null +++ b/openspec/changes/guards-refuse-when-unsure/tasks.md @@ -0,0 +1,30 @@ +# Tasks: guards-refuse-when-unsure + +Decision 224. One PR, `--base development`, stacked on the method-decomposition slices 8e to 8g. + +## 1. One refusal for a check that could not run + +- [x] 1.1 (`lib/Service/Zgw/ZgwRuleResults.php`) `notVerified()` answers 503 `check-not-completed` with the rule in the detail and logs the rule and the object at error (REQ-ZBR-020). + - `tests/Unit/Service/Zgw/ZgwRuleResultsTest.php`: "a check that could not run refuses and is logged". Control: an ordinary refusal logs nothing. +- [x] 1.2 (`lib/Service/Zgw/ZgwRuleLookups.php`) `idsByField()` throws when the search cannot run. `findAllObjectsByField()` keeps its empty answer for the reference rules that are not part of this change. + +## 2. The document rules + +- [x] 2.1 (`lib/Service/Zgw/DrcObjectLinkRules.php`) drc-004 refuses when the counterpart lookup fails (REQ-ZBR-020). +- [x] 2.2 (`lib/Service/Zgw/DrcDocumentRelations.php`, `lib/Service/ZgwDrcRulesService.php`) drc-007 refuses when the relation search fails. +- [x] 2.3 (`lib/Service/Zgw/DrcDocumentRelations.php`) drc-003 refuses with the same answer and no longer claims a duplicate. +- [x] 2.4 (`lib/Controller/DrcController.php`) the relation check before a document delete (drc-008a) refuses when one of its three searches fails. + - Each with a control in the same test: the same search answering lets the request through, or answers the rule's own refusal. + +## 3. The case type delete check + +- [x] 3.1 (`lib/Service/Zgw/ZtcPublishAndDeleteChecks.php`, `lib/Service/ZgwBusinessRulesService.php`) cases or final statuses that cannot be read block the delete as unverified; the ZGW delete answers 503 `destroy_check_not_completed` (REQ-ZBR-021). + - `tests/Unit/Service/ZgwBusinessRulesServiceDestroyGuardTest.php` asserts it from the caller. + +## 4. The debt list + +- [x] 4.1 (`tests/Unit/Architecture/catch-return-null.allowlist.json`) two sites left, ceiling 263 to 261. + +## 5. Live + +- [ ] 5.1 (live pass, decision 139) With the store reachable: an objectinformatieobject create, a document delete and a case type delete answer as before. diff --git a/tests/Unit/Architecture/catch-return-null.allowlist.json b/tests/Unit/Architecture/catch-return-null.allowlist.json index d94eea4848..372f972f23 100644 --- a/tests/Unit/Architecture/catch-return-null.allowlist.json +++ b/tests/Unit/Architecture/catch-return-null.allowlist.json @@ -1,8 +1,9 @@ { - "ceiling": 263, + "ceiling": 261, "measured": "2026-09-19", "window": 3, "about": [ + "2026-10-12, decision 224 (guards refuse when unsure): two sites left. `Zgw/DrcDocumentRelations::findOioRelationsForDocument` (drc-007) and `Zgw/ZtcPublishAndDeleteChecks::loadFinalStatusSlugs` (the case type delete check) throw when the store cannot be read, and their rule refuses with 503 `check-not-completed` instead of reading \"none\". The ceiling went 263 to 261.", "2026-10-12, openspec/changes/retire-cmmn-caseplanstate task 3.1: the local CMMN runtime retired, taking `Cmmn/CaseModelLoader::getActiveModel` with it. The ceiling went 264 to 263.", "2026-10-10, openspec/changes/remove-casetask task 7.1: onboarding steps moved onto the engine. OnboardingSteps reads through EngineInboxQuery and resolves its services inside each verb's own try block, so it adds no site; TenantOnboardingService::markStepComplete went, legacyRows (the one-off carry of the old rows) arrived. The ceiling stays 257.", "2026-10-10, openspec/changes/tenancy-onto-openregister-organisation tasks 6.9 and 6.15: TenantSaasService retired with the tenant admin store, taking its three sites (getById, getObjectService, listActive). TenantService::getObjectService arrived with the audit anchor. The ceiling went 259 to 257.", @@ -1734,13 +1735,6 @@ "class": "read-miss", "reason": "`contentFor()` answers `return null` when the read fails; the catch logs nothing, so a failed read and an absent record are the same answer." }, - { - "file": "lib/Service/Zgw/DrcDocumentRelations.php", - "method": "findOioRelationsForDocument", - "occurrence": 1, - "class": "read-miss", - "reason": "`findOioRelationsForDocument()` answers `return []` when the read fails; the catch logs at warning (\"Drc-007: OIO relation search with partial match failed\"), so a failed read and an absent record are the same answer." - }, { "file": "lib/Service/ZgwRulesBase.php", "method": "fetchExternalUrl", @@ -1790,13 +1784,6 @@ "class": "read-miss", "reason": "`loadCaseTypeRow()` answers `return []` when the read fails; the catch logs nothing, so a failed read and an absent record are the same answer." }, - { - "file": "lib/Service/Zgw/ZtcPublishAndDeleteChecks.php", - "method": "loadFinalStatusSlugs", - "occurrence": 1, - "class": "read-miss", - "reason": "`loadFinalStatusSlugs()` answers `return []` when the read fails; the catch logs nothing, so a failed read and an absent record are the same answer." - }, { "file": "lib/Service/Flow/RetiredActionRunner.php", "method": "registry", diff --git a/tests/Unit/Controller/DrcEioLifecycleTest.php b/tests/Unit/Controller/DrcEioLifecycleTest.php index f1e364bbd2..044cba8f42 100644 --- a/tests/Unit/Controller/DrcEioLifecycleTest.php +++ b/tests/Unit/Controller/DrcEioLifecycleTest.php @@ -56,6 +56,7 @@ public function saveObject(mixed $register, mixed $schema, array $object, ?strin * Characterisation tests for the EIO delete, update and chunked create. * * @covers \OCA\Dossiq\Controller\DrcController + * @uses \OCA\Dossiq\Service\Zgw\ZgwRuleResults * @uses \OCA\Dossiq\Service\Zgw\ZgwSearchScope * @uses \OCA\Dossiq\Support\NormalisesObjectRows */ @@ -106,7 +107,14 @@ static function (mixed $id, mixed $register, mixed $schema) use ($stored): mixed static fn (array $requestParams, mixed $register, mixed $schema): array => ['schema' => (string)$schema, 'params' => $requestParams] ); $objects->method('searchObjectsPaginated')->willReturnCallback( - static fn (array $query): array => ['results' => $searches[$query['schema']] ?? []] + static function (array $query) use ($searches): array { + $answer = ($searches[$query['schema']] ?? []); + if ($answer instanceof \Throwable) { + throw $answer; + } + + return ['results' => $answer]; + } ); $objects->method('deleteObject')->willReturnCallback(function (string $uuid): bool { $this->seen['deleted'][] = $uuid; @@ -135,6 +143,9 @@ function ($request, string $api, string $resource, string $uuid) use ($destroySt return new JSONResponse(data: [], statusCode: $destroyStatus); } ); + $zgw->method('buildValidationError')->willReturnCallback( + static fn (array $ruleResult): array => ['detail' => $ruleResult['detail'], 'code' => ($ruleResult['code'] ?? '')] + ); $zgw->method('getRequestBody')->willReturn($body); $rules = $this->createMock(ZgwBusinessRulesService::class); $rules->method('validate')->willReturn(['valid' => true, 'enrichedBody' => $body]); @@ -175,6 +186,27 @@ public function testDocumentWithARelationIsNotDeleted(): void { }//end testDocumentWithARelationIsNotDeleted() + /** + * A document whose relations could not be looked up is not deleted either: 503, nothing cascades. + * + * The besluit side fails here while the other two answer none. Control: testDeletedDocumentCascades, + * where every search answers and the delete goes through. + * + * @return void + */ + public function testDocumentWhoseRelationsCannotBeReadIsNotDeleted(): void { + $response = $this->controller(stored: ['id' => self::UUID], searches: ['14' => new \RuntimeException('down'), '15' => [['id' => 'gr-1']]]) + ->destroy('enkelvoudiginformatieobjecten', self::UUID); + + $this->assertSame(Http::STATUS_SERVICE_UNAVAILABLE, $response->getStatus()); + $this->assertSame('check-not-completed', $response->getData()['code']); + $this->assertStringContainsString('drc-008a', $response->getData()['detail']); + $this->assertSame([], $this->seen['destroyed']); + $this->assertSame([], $this->seen['deleted']); + $this->assertSame([], $this->seen['files']); + + }//end testDocumentWhoseRelationsCannotBeReadIsNotDeleted() + /** * A deleted document takes its gebruiksrechten and its stored files with it. * diff --git a/tests/Unit/Service/Zgw/DrcDocumentRelationsTest.php b/tests/Unit/Service/Zgw/DrcDocumentRelationsTest.php index 28940b8a21..04f7b273e4 100644 --- a/tests/Unit/Service/Zgw/DrcDocumentRelationsTest.php +++ b/tests/Unit/Service/Zgw/DrcDocumentRelationsTest.php @@ -68,11 +68,11 @@ class DrcDocumentRelationsTest extends TestCase { private array $queries = []; /** - * The warnings the relations logged. + * The errors the relations logged: message and context. * - * @var array + * @var array */ - private array $warnings = []; + private array $errors = []; /** * Build the relations over a search that answers from its query. @@ -88,9 +88,9 @@ private function relations(\Closure $answer, array $mappingConfig = ['sourceRegi $settings->method('getConfigValue')->willReturnCallback(static fn (string $key): string => ($schemas[$key] ?? $key)); $logger = $this->createMock(LoggerInterface::class); - $logger->method('warning')->willReturnCallback( - function (string $message): void { - $this->warnings[] = $message; + $logger->method('error')->willReturnCallback( + function (string $message, array $context = []): void { + $this->errors[] = [$message, $context]; } ); @@ -110,10 +110,9 @@ function (array $query) use ($answer): array { return new DrcDocumentRelations( lookups: $lookups, - results: new ZgwRuleResults(), + results: new ZgwRuleResults(logger: $logger), settingsService: $settings, - fieldValidator: new FieldValidator(), - logger: $logger + fieldValidator: new FieldValidator() ); }//end relations() @@ -157,20 +156,38 @@ public function jsonSerialize(): array { }//end testRelationsStoredByUrlAreFoundByPartialMatch() /** - * A search that fails answers no relations and says so in the log. + * A relation search that cannot run throws: "could not look" is not "no relations". + * + * Control: the same search answering nothing is an empty list. * * @return void */ - public function testAFailedRelationSearchAnswersNone(): void { - $answer = static function (array $query): array { + public function testAFailedRelationSearchThrows(): void { + $none = static fn (array $query): array => ['results' => []]; + $down = static function (array $query): array { throw new \RuntimeException('down'); }; + $partialDown = static function (array $query): array { + if (str_starts_with($query['document'], '%') === true) { + throw new \RuntimeException('partial down'); + } - $ids = $this->relations(answer: $answer)->findOioRelationsForDocument(register: '5', schema: '9', docUuid: self::DOCUMENT); + return ['results' => []]; + }; + + $this->assertSame([], $this->relations(answer: $none)->findOioRelationsForDocument(register: '5', schema: '9', docUuid: self::DOCUMENT)); - $this->assertSame([], $ids); - $this->assertSame('Drc-007: OIO relation search with partial match failed: down', $this->warnings[1]); - }//end testAFailedRelationSearchAnswersNone() + $thrown = []; + foreach ([$down, $partialDown] as $answer) { + try { + $this->relations(answer: $answer)->findOioRelationsForDocument(register: '5', schema: '9', docUuid: self::DOCUMENT); + } catch (\RuntimeException $e) { + $thrown[] = $e->getMessage(); + } + } + + $this->assertSame(['down', 'partial down'], $thrown); + }//end testAFailedRelationSearchThrows() /** * A pair nobody stored is unique after the link schema and the besluit's own schema are both read. @@ -258,18 +275,30 @@ public function testAMissingRegisterOrSchemaIsNotSearched(): void { }//end testAMissingRegisterOrSchemaIsNotSearched() /** - * A duplicate search that cannot answer counts as a duplicate and is logged. + * Drc-003: a duplicate search that cannot run refuses, and does not claim a duplicate nobody found. + * + * Control: the same search answering nothing passes, so the refusal comes from the failure. * * @return void */ - public function testAFailedDuplicateSearchFailsClosed(): void { - $answer = static function (array $query): array { + public function testAFailedDuplicateSearchRefusesAsNotVerified(): void { + $none = static fn (array $query): array => ['total' => 0]; + $down = static function (array $query): array { throw new \RuntimeException('down'); }; - $error = $this->relations(answer: $answer)->checkOioUniqueness(ioUrl: self::DOCUMENT_URL, objectUrl: self::BESLUIT_URL, objectType: 'case'); + $this->assertNull($this->relations(answer: $none)->checkOioUniqueness(ioUrl: self::DOCUMENT_URL, objectUrl: self::BESLUIT_URL, objectType: 'case')); + $this->assertSame([], $this->errors); - $this->assertSame('unique', $error['invalidParams'][0]['code']); - $this->assertSame(['Drc-003: Uniqueness check failed for schema {schema}: {message}'], $this->warnings); - }//end testAFailedDuplicateSearchFailsClosed() + $error = $this->relations(answer: $down)->checkOioUniqueness(ioUrl: self::DOCUMENT_URL, objectUrl: self::BESLUIT_URL, objectType: 'case'); + + $this->assertSame(503, $error['status']); + $this->assertSame('check-not-completed', $error['code']); + $this->assertStringContainsString('drc-003', $error['detail']); + $this->assertSame([], $error['invalidParams']); + $this->assertSame( + ['rule' => 'drc-003', 'object' => 'document ' . self::DOCUMENT_URL . ' on ' . self::BESLUIT_URL, 'cause' => 'down'], + $this->errors[0][1] + ); + }//end testAFailedDuplicateSearchRefusesAsNotVerified() }//end class diff --git a/tests/Unit/Service/Zgw/DrcObjectLinkRulesTest.php b/tests/Unit/Service/Zgw/DrcObjectLinkRulesTest.php index a7e432a484..a412a68777 100644 --- a/tests/Unit/Service/Zgw/DrcObjectLinkRulesTest.php +++ b/tests/Unit/Service/Zgw/DrcObjectLinkRulesTest.php @@ -69,6 +69,13 @@ class DrcObjectLinkRulesTest extends TestCase { */ private array $queries = []; + /** + * The context of every error the rules logged. + * + * @var array + */ + private array $errors = []; + /** * Build the rules, with a search that answers from its query when one is given. * @@ -84,7 +91,12 @@ private function rules(?\Closure $answer, array $mappingConfig = ['sourceRegiste $logger = $this->createMock(LoggerInterface::class); $lookups = new ZgwRuleLookups(logger: $logger, settingsService: $settings); - $parts = ['lookups' => $lookups, 'results' => new ZgwRuleResults(), 'settingsService' => $settings, 'fieldValidator' => new FieldValidator(), 'logger' => $logger]; + $logger->method('error')->willReturnCallback( + function (string $message, array $context = []): void { + $this->errors[] = $context; + } + ); + $parts = ['lookups' => $lookups, 'results' => new ZgwRuleResults(logger: $logger), 'settingsService' => $settings, 'fieldValidator' => new FieldValidator()]; $rules = new DrcObjectLinkRules(...$parts, relations: new DrcDocumentRelations(...$parts)); if ($answer === null) { return $rules; @@ -241,11 +253,14 @@ public function testAMissingCounterpartIsInconsistent(): void { }//end testAMissingCounterpartIsInconsistent() /** - * Drc-004 passes when the counterpart is found, and when its lookup fails. + * Drc-004 passes when the counterpart is found, and refuses when its lookup could not run. + * + * The found counterpart is the control: the refusal comes from the failed lookup, not from the body. + * A lookup that ran and found nothing is the third answer, inconsistent-relation (the test above). * * @return void */ - public function testAFoundCounterpartOrAFailedLookupPasses(): void { + public function testAFoundCounterpartPassesAndAFailedLookupRefuses(): void { $body = ['informatieobject' => self::DOCUMENT_URL, 'object' => self::CASE_URL, 'objectType' => 'case']; $found = static fn (array $query): array => ['total' => (int)self::isCounterpartLookup(query: $query)]; $down = static function (array $query): array { @@ -257,8 +272,26 @@ public function testAFoundCounterpartOrAFailedLookupPasses(): void { }; $this->assertTrue($this->rules(answer: $found)->rulesObjectinformatieobjectenCreate(body: $body)['valid']); - $this->assertTrue($this->rules(answer: $down)->rulesObjectinformatieobjectenCreate(body: $body)['valid']); - }//end testAFoundCounterpartOrAFailedLookupPasses() + $this->assertSame([], $this->errors); + + $refused = $this->rules(answer: $down)->rulesObjectinformatieobjectenCreate(body: $body); + + $this->assertSame( + [ + 'valid' => false, + 'status' => 503, + 'detail' => 'De controle van regel drc-004 kon niet worden uitgevoerd. Er is niets gewijzigd. Probeer het later opnieuw.', + 'invalidParams' => [], + 'enrichedBody' => [], + 'code' => 'check-not-completed', + ], + $refused + ); + $this->assertSame( + [['rule' => 'drc-004', 'object' => 'case ' . self::CASE . ' with document ' . self::DOCUMENT, 'cause' => 'down']], + $this->errors + ); + }//end testAFoundCounterpartPassesAndAFailedLookupRefuses() /** * Drc-004 is skipped without an objectType, for an unknown one, and without a schema or register. diff --git a/tests/Unit/Service/Zgw/ZgwRuleLookupsTest.php b/tests/Unit/Service/Zgw/ZgwRuleLookupsTest.php index 0ae2fed6fb..d86b604a7b 100644 --- a/tests/Unit/Service/Zgw/ZgwRuleLookupsTest.php +++ b/tests/Unit/Service/Zgw/ZgwRuleLookupsTest.php @@ -179,6 +179,37 @@ public function testEveryObjectByField(): void { $this->assertSame(['Reference resolution failed: down'], $this->warnings); }//end testEveryObjectByField() + /** + * The strict read tells "none" from "could not look": an empty answer is an empty list, a search + * that fails throws, and so does a read without a store. + * + * @return void + */ + public function testTheStrictReadThrowsWhenItCannotLook(): void { + $some = $this->store(search: static fn (array $query): array => ['results' => [['id' => 'a']]]); + $none = $this->store(search: static fn (array $query): array => ['results' => []]); + $down = $this->store( + search: static function (): array { + throw new \RuntimeException('down'); + } + ); + + $this->assertSame(['a'], $this->lookups(store: $some)->idsByField(register: '5', schema: '9', field: 'document', value: 'x')); + $this->assertSame([], $this->lookups(store: $none)->idsByField(register: '5', schema: '9', field: 'document', value: 'x')); + + $thrown = []; + foreach ([$down, null] as $store) { + try { + $this->lookups(store: $store)->idsByField(register: '5', schema: '9', field: 'document', value: 'x'); + } catch (\RuntimeException $e) { + $thrown[] = $e->getMessage(); + } + } + + $this->assertSame(['down', 'No object service in the request context'], $thrown); + $this->assertSame([], $this->warnings, 'the strict read leaves the logging to the rule that refuses'); + }//end testTheStrictReadThrowsWhenItCannotLook() + /** * An object by uuid is read in the request's register and the schema the key names. * diff --git a/tests/Unit/Service/Zgw/ZgwRuleResultsTest.php b/tests/Unit/Service/Zgw/ZgwRuleResultsTest.php index f5a972beb4..f3d9ad9926 100644 --- a/tests/Unit/Service/Zgw/ZgwRuleResultsTest.php +++ b/tests/Unit/Service/Zgw/ZgwRuleResultsTest.php @@ -24,6 +24,7 @@ use OCA\Dossiq\Service\Zgw\ZgwRuleResults; use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; /** * The valid and the refused result of a rule. @@ -33,6 +34,63 @@ * @spec openspec/changes/method-decomposition/tasks.md */ final class ZgwRuleResultsTest extends TestCase { + /** + * What the builder logged at error: message and context. + * + * @var array + */ + private array $errors = []; + + /** + * The builder over a logger that records its errors. + * + * @return ZgwRuleResults + */ + private function results(): ZgwRuleResults { + $logger = $this->createMock(LoggerInterface::class); + $logger->method('error')->willReturnCallback( + function (string $message, array $context = []): void { + $this->errors[] = [$message, $context]; + } + ); + + return new ZgwRuleResults(logger: $logger); + }//end results() + + /** + * A check that could not run refuses with a 503, names the rule, blames no field, and is logged + * with the rule and the object. + * + * Control: an ordinary refusal logs nothing and keeps its own status. + * + * @return void + */ + public function testACheckThatCouldNotRunRefusesAndIsLogged(): void { + $results = $this->results(); + + $results->error(status: 400, detail: 'Nee.'); + $this->assertSame([], $this->errors); + + $this->assertSame( + [ + 'valid' => false, + 'status' => 503, + 'detail' => 'De controle van regel drc-004 kon niet worden uitgevoerd. Er is niets gewijzigd. Probeer het later opnieuw.', + 'invalidParams' => [], + 'enrichedBody' => [], + 'code' => 'check-not-completed', + ], + $results->notVerified(rule: 'drc-004', object: 'case c-1 with document d-1', cause: 'down') + ); + $this->assertSame( + [[ + 'ZGW rule {rule} could not be checked for {object}, so the request is refused: {cause}', + ['rule' => 'drc-004', 'object' => 'case c-1 with document d-1', 'cause' => 'down'], + ]], + $this->errors + ); + }//end testACheckThatCouldNotRunRefusesAndIsLogged() + /** * A pass carries the body and a 200. * @@ -41,7 +99,7 @@ final class ZgwRuleResultsTest extends TestCase { public function testAPassCarriesTheBody(): void { $this->assertSame( ['valid' => true, 'status' => 200, 'detail' => '', 'enrichedBody' => ['a' => 1]], - (new ZgwRuleResults())->isValid(body: ['a' => 1]) + $this->results()->isValid(body: ['a' => 1]) ); }//end testAPassCarriesTheBody() @@ -51,7 +109,7 @@ public function testAPassCarriesTheBody(): void { * @return void */ public function testARefusalCarriesItsFields(): void { - $results = new ZgwRuleResults(); + $results = $this->results(); $field = $results->fieldError(fieldName: 'zaaktype', code: 'bad-url', reason: 'Ongeldig.'); $this->assertSame(['name' => 'zaaktype', 'code' => 'bad-url', 'reason' => 'Ongeldig.'], $field); @@ -81,7 +139,7 @@ public function testAnImmutableFieldIsRefusedByName(): void { 'invalidParams' => [['name' => 'identificatie', 'code' => 'wijzigen-niet-toegelaten', 'reason' => $detail]], 'enrichedBody' => [], ], - (new ZgwRuleResults())->fieldImmutableError(fieldName: 'identificatie') + $this->results()->fieldImmutableError(fieldName: 'identificatie') ); }//end testAnImmutableFieldIsRefusedByName() }//end class diff --git a/tests/Unit/Service/Zgw/ZtcConceptRulesTest.php b/tests/Unit/Service/Zgw/ZtcConceptRulesTest.php index 7ce90ac172..93221e9577 100644 --- a/tests/Unit/Service/Zgw/ZtcConceptRulesTest.php +++ b/tests/Unit/Service/Zgw/ZtcConceptRulesTest.php @@ -41,7 +41,7 @@ class ZtcConceptRulesTest extends TestCase { * @return ZtcConceptRules */ private function rules(): ZtcConceptRules { - return new ZtcConceptRules(results: new ZgwRuleResults()); + return new ZtcConceptRules(results: new ZgwRuleResults(logger: $this->createMock(\Psr\Log\LoggerInterface::class))); }//end rules() /** diff --git a/tests/Unit/Service/Zgw/ZtcPublishAndDeleteChecksTest.php b/tests/Unit/Service/Zgw/ZtcPublishAndDeleteChecksTest.php index 39e504d4fc..7a197ecf12 100644 --- a/tests/Unit/Service/Zgw/ZtcPublishAndDeleteChecksTest.php +++ b/tests/Unit/Service/Zgw/ZtcPublishAndDeleteChecksTest.php @@ -24,6 +24,7 @@ use OCA\Dossiq\Service\SettingsService; use OCA\Dossiq\Service\Zgw\ZgwRuleLookups; +use OCA\Dossiq\Service\Zgw\ZgwRuleResults; use OCA\Dossiq\Service\Zgw\ZtcPublishAndDeleteChecks; use OCP\AppFramework\Db\DoesNotExistException; use PHPUnit\Framework\TestCase; @@ -34,6 +35,7 @@ * * @covers \OCA\Dossiq\Service\Zgw\ZtcPublishAndDeleteChecks * @uses \OCA\Dossiq\Service\Zgw\ZgwRuleLookups + * @uses \OCA\Dossiq\Service\Zgw\ZgwRuleResults * @uses \OCA\Dossiq\Service\Support\SearchesObjects * @uses \OCA\Dossiq\Service\DeadlineMonitoringSeedDataService * @@ -42,6 +44,20 @@ class ZtcPublishAndDeleteChecksTest extends TestCase { private const DEFAULT = ['blocked' => false, 'requiresConfirmation' => false, 'message' => '']; + private const UNVERIFIED = [ + 'blocked' => true, + 'requiresConfirmation' => false, + 'unverified' => true, + 'message' => 'Cannot delete case type: the check for cases that use this type could not be completed. Nothing was deleted. Try again later.', + ]; + + /** + * The context of every error logged. + * + * @var array + */ + private array $errors = []; + /** * Build the checks over a store answering searches per schema and one case type row. * @@ -54,8 +70,14 @@ private function checks(?array $rows, array|\Throwable|null $caseType = null): Z $settings = $this->createMock(SettingsService::class); $settings->method('getConfigValue')->willReturnCallback(static fn (string $key): string => $key); - $lookups = new ZgwRuleLookups(logger: $this->createMock(LoggerInterface::class), settingsService: $settings); - $checks = new ZtcPublishAndDeleteChecks(lookups: $lookups, settingsService: $settings); + $logger = $this->createMock(LoggerInterface::class); + $logger->method('error')->willReturnCallback( + function (string $message, array $context = []): void { + $this->errors[] = $context; + } + ); + $lookups = new ZgwRuleLookups(logger: $logger, settingsService: $settings); + $checks = new ZtcPublishAndDeleteChecks(lookups: $lookups, results: new ZgwRuleResults(logger: $logger), settingsService: $settings); if ($rows === null) { return $checks; } @@ -157,22 +179,46 @@ public function testThePublishCheckStopsWhenItCannotRun(): void { }//end testThePublishCheckStopsWhenItCannotRun() /** - * A case type nobody uses, or one whose use cannot be read, may be deleted. + * A case type nobody uses may be deleted, and without an id there is nothing to check. * * @return void */ public function testAnUnusedCaseTypeMayBeDeleted(): void { $this->assertSame(self::DEFAULT, $this->checks(rows: [])->validateDeletion(register: 'reg', caseTypeId: 'ct-1')); - $this->assertSame(self::DEFAULT, $this->checks(rows: null)->validateDeletion(register: 'reg', caseTypeId: 'ct-1')); $this->assertSame(self::DEFAULT, $this->checks(rows: ['case_schema' => [['status' => 'open']]])->validateDeletion(register: 'reg', caseTypeId: '')); + $this->assertSame([], $this->errors); + }//end testAnUnusedCaseTypeMayBeDeleted() + + /** + * A delete check that cannot run blocks the delete as unverified, and the log names the check and + * the case type: no store, cases that cannot be read, final statuses that cannot be read. + * + * Control: testAnUnusedCaseTypeMayBeDeleted, where the same read answers no cases. + * + * @return void + */ + public function testACheckThatCannotRunBlocksTheDelete(): void { + $down = new \RuntimeException('down'); + + $this->assertSame(self::UNVERIFIED, $this->checks(rows: null)->validateDeletion(register: 'reg', caseTypeId: 'ct-1')); + $this->assertSame(self::UNVERIFIED, $this->checks(rows: ['case_schema' => $down])->validateDeletion(register: 'reg', caseTypeId: 'ct-1')); $this->assertSame( - self::DEFAULT, - $this->checks(rows: ['case_schema' => new \RuntimeException('down')])->validateDeletion(register: 'reg', caseTypeId: 'ct-1') + self::UNVERIFIED, + $this->checks(rows: ['case_schema' => [['status' => 'final']], 'status_type_schema:final' => $down]) + ->validateDeletion(register: 'reg', caseTypeId: 'ct-1') ); - }//end testAnUnusedCaseTypeMayBeDeleted() + $this->assertSame( + [ + ['rule' => 'ztc case type delete check (REQ-CT-01d)', 'object' => 'case type ct-1', 'cause' => 'No object service in the request context'], + ['rule' => 'ztc case type delete check (REQ-CT-01d)', 'object' => 'case type ct-1', 'cause' => 'down'], + ['rule' => 'ztc case type delete check (REQ-CT-01d)', 'object' => 'case type ct-1', 'cause' => 'down'], + ], + $this->errors + ); + }//end testACheckThatCannotRunBlocksTheDelete() /** - * Active cases block the delete; a case is closed only on a final status that could be read. + * Active cases block the delete; a case is closed only on a final status. * * @return void */ @@ -188,11 +234,6 @@ public function testActiveCasesBlockTheDelete(): void { $this->checks(rows: ['case_schema' => $cases, 'status_type_schema:final' => [['id' => 'final'], 'no row', ['name' => 'no id']]]) ->validateDeletion(register: 'reg', caseTypeId: 'ct-1') ); - $this->assertStringContainsString( - '4 active case(s)', - $this->checks(rows: ['case_schema' => $cases, 'status_type_schema:final' => new \RuntimeException('down')]) - ->validateDeletion(register: 'reg', caseTypeId: 'ct-1')['message'] - ); }//end testActiveCasesBlockTheDelete() /** diff --git a/tests/Unit/Service/ZgwBusinessRulesServiceDestroyGuardTest.php b/tests/Unit/Service/ZgwBusinessRulesServiceDestroyGuardTest.php new file mode 100644 index 0000000000..77fdd8aadd --- /dev/null +++ b/tests/Unit/Service/ZgwBusinessRulesServiceDestroyGuardTest.php @@ -0,0 +1,116 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * @version GIT: + * + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\Dossiq\Tests\Unit\Service; + +use OCA\Dossiq\Service\Zgw\ZgwRulesDispatcher; +use OCA\Dossiq\Service\ZgwBusinessRulesService; +use OCA\Dossiq\Service\ZgwZtcRulesService; +use PHPUnit\Framework\TestCase; + +/** + * What the delete of a case type answers per verdict of the delete check. + * + * @covers \OCA\Dossiq\Service\ZgwBusinessRulesService + * @uses \OCA\Dossiq\Service\Zgw\ZgwRuleResults + * + * @spec openspec/specs/zgw-business-rules-compliance/spec.md + */ +class ZgwBusinessRulesServiceDestroyGuardTest extends TestCase { + /** + * How often the delete went on to the per-register rules. + * + * @var int + */ + private int $dispatched = 0; + + /** + * The answer of deleting case type ct-1 for one verdict of the delete check. + * + * @param array $verdict What the delete check answers. + * @param array $body The request body. + * + * @return array + */ + private function destroy(array $verdict, array $body = []): array { + $ztc = $this->createMock(ZgwZtcRulesService::class); + $ztc->method('checkConceptProtection')->willReturn(null); + $ztc->method('validateDeletion')->with('reg', 'ct-1')->willReturn($verdict); + + $dispatcher = $this->createMock(ZgwRulesDispatcher::class); + $dispatcher->method('dispatch')->willReturnCallback( + function () use ($body): array { + $this->dispatched++; + return ['valid' => true, 'status' => 200, 'detail' => '', 'enrichedBody' => $body]; + } + ); + + return (new ZgwBusinessRulesService(ztcRules: $ztc, dispatcher: $dispatcher))->validate( + zgwApi: 'catalogi', + resource: 'zaaktypen', + action: 'destroy', + body: $body, + existingObject: ['id' => 'ct-1'], + objectService: new \stdClass(), + mappingConfig: ['sourceRegister' => 'reg'] + ); + }//end destroy() + + /** + * A delete check that could not run refuses with a 503 and its own code, and the delete does not + * go on, also when the caller confirmed. + * + * Control: a check that ran and found no cases lets the delete go on. + * + * @return void + */ + public function testACheckThatCouldNotRunRefusesTheDelete(): void { + $free = $this->destroy(verdict: ['blocked' => false, 'requiresConfirmation' => false, 'message' => '']); + $this->assertTrue($free['valid']); + $this->assertSame(1, $this->dispatched); + + $unverified = ['blocked' => true, 'requiresConfirmation' => false, 'unverified' => true, 'message' => 'Could not be completed.']; + $this->assertSame( + ['valid' => false, 'status' => 503, 'detail' => 'Could not be completed.', 'code' => 'destroy_check_not_completed', 'enrichedBody' => ['_confirm' => true]], + $this->destroy(verdict: $unverified, body: ['_confirm' => true]) + ); + $this->assertSame(1, $this->dispatched, 'a refused delete does not reach the per-register rules'); + }//end testACheckThatCouldNotRunRefusesTheDelete() + + /** + * Active cases stay a conflict, and closed cases ask for confirmation once. + * + * @return void + */ + public function testActiveAndClosedCasesKeepTheirAnswers(): void { + $active = $this->destroy(verdict: ['blocked' => true, 'requiresConfirmation' => false, 'message' => '3 active.']); + $closed = ['blocked' => false, 'requiresConfirmation' => true, 'message' => '2 closed.']; + $asked = $this->destroy(verdict: $closed); + + $this->assertSame([409, 'destroy_blocked_active_cases'], [$active['status'], $active['code']]); + $this->assertSame([409, 'destroy_requires_confirmation'], [$asked['status'], $asked['code']]); + $this->assertTrue($this->destroy(verdict: $closed, body: ['_confirm' => true])['valid']); + }//end testActiveAndClosedCasesKeepTheirAnswers() +}//end class diff --git a/tests/Unit/Service/ZgwDrcRulesServiceTest.php b/tests/Unit/Service/ZgwDrcRulesServiceTest.php index 2a4e3b59c5..5e62c15bd8 100644 --- a/tests/Unit/Service/ZgwDrcRulesServiceTest.php +++ b/tests/Unit/Service/ZgwDrcRulesServiceTest.php @@ -470,11 +470,13 @@ public function jsonSerialize(): array { }//end serialisable() /** - * Drc-007: no relation, or a relation search that fails, lets the delete through. + * Drc-007: no relation lets the delete through; a relation search that could not run refuses it. + * + * The delete without a relation is the control: the search ran and found none. * * @return void */ - public function testDestroyPassesWithoutARelationOrWhenTheSearchFails(): void { + public function testDestroyPassesWithoutARelationAndRefusesWhenTheSearchFails(): void { $none = $this->buildService(objects: []) ->rulesEnkelvoudiginformatieobjectenDestroy(body: ['x' => 1], existingObject: ['id' => self::IO_UUID]); @@ -486,9 +488,13 @@ public function testDestroyPassesWithoutARelationOrWhenTheSearchFails(): void { $this->assertTrue($none['valid']); $this->assertSame(['x' => 1], $none['enrichedBody']); - $this->assertTrue($failed['valid']); + $this->assertFalse($failed['valid']); + $this->assertSame(503, $failed['status']); + $this->assertSame('check-not-completed', $failed['code']); + $this->assertStringContainsString('drc-007', $failed['detail']); + $this->assertSame([], $failed['invalidParams']); - }//end testDestroyPassesWithoutARelationOrWhenTheSearchFails() + }//end testDestroyPassesWithoutARelationAndRefusesWhenTheSearchFails() /** * Drc-003: a relation stored by uuid, or found only by full text, is a duplicate too. @@ -545,29 +551,38 @@ public function testRelationInZrcOrBrcIsADuplicate(): void { }//end testRelationInZrcOrBrcIsADuplicate() /** - * Drc-003 fails closed: a duplicate search that cannot answer counts as a duplicate. + * Drc-003: a duplicate search that could not run refuses, and claims no duplicate. + * + * Control: testAKnownObjectTypeNeverPassesBothRelationRules runs the same body over a search that answers. * * @return void */ - public function testFailedDuplicateSearchCountsAsADuplicate(): void { + public function testFailedDuplicateSearchRefusesAsNotVerified(): void { $this->searchHandler = static function (array $query): array { throw new \RuntimeException('down'); }; $result = $this->buildService(objects: [])->rulesObjectinformatieobjectenCreate(body: $this->caseLink()); - $this->assertSame('unique', $result['invalidParams'][0]['code']); + $this->assertSame(503, $result['status']); + $this->assertSame('check-not-completed', $result['code']); + $this->assertStringContainsString('drc-003', $result['detail']); + $this->assertSame([], $result['invalidParams']); - }//end testFailedDuplicateSearchCountsAsADuplicate() + }//end testFailedDuplicateSearchRefusesAsNotVerified() /** - * Drc-004 fails open: a cross-register search that cannot answer lets the relation through. + * Drc-004: a cross-register search that could not run refuses the relation. * - * The cross-register search is the one that names the case before the document. + * The cross-register search is the one that names the case before the document. Control: the + * same search finding the counterpart passes. * * @return void */ - public function testFailedCrossRegisterSearchPasses(): void { + public function testFailedCrossRegisterSearchRefuses(): void { + $this->searchHandler = static fn (array $query): array => ['total' => (int)(array_key_first($query) === 'case')]; + $found = $this->buildService(objects: [])->rulesObjectinformatieobjectenCreate(body: $this->caseLink()); + $this->searchHandler = static function (array $query): array { if (array_key_first($query) === 'case') { throw new \RuntimeException('down'); @@ -575,12 +590,15 @@ public function testFailedCrossRegisterSearchPasses(): void { return ['total' => 0]; }; - $result = $this->buildService(objects: [])->rulesObjectinformatieobjectenCreate(body: $this->caseLink()); - $this->assertTrue($result['valid']); + $this->assertTrue($found['valid']); + $this->assertFalse($result['valid']); + $this->assertSame(503, $result['status']); + $this->assertSame('check-not-completed', $result['code']); + $this->assertStringContainsString('drc-004', $result['detail']); - }//end testFailedCrossRegisterSearchPasses() + }//end testFailedCrossRegisterSearchRefuses() /** * Drc-003 and drc-004 read the same ZaakInformatieObject: present it is a duplicate, absent it is diff --git a/tests/Unit/Service/ZgwZtcRulesServiceTest.php b/tests/Unit/Service/ZgwZtcRulesServiceTest.php index e9d8851555..1b3faf2288 100644 --- a/tests/Unit/Service/ZgwZtcRulesServiceTest.php +++ b/tests/Unit/Service/ZgwZtcRulesServiceTest.php @@ -220,10 +220,14 @@ public function testValidateDeletionRequiresConfirmationWhenOnlyClosedCases(): v $this->assertStringContainsString('1 closed case(s)', $r['message']); } - public function testValidateDeletionReturnsDefaultWithoutObjectService(): void { + /** + * Without an object service the delete check cannot run, so it blocks as unverified (decision 224). + */ + public function testValidateDeletionBlocksWithoutObjectService(): void { $this->svc->setContext(null, []); $r = $this->svc->validateDeletion('reg', 'ct-1'); - $this->assertFalse($r['blocked']); + $this->assertTrue($r['blocked']); + $this->assertTrue($r['unverified']); $this->assertFalse($r['requiresConfirmation']); } @@ -318,22 +322,36 @@ public function testValidatePublishReadsTheConfiguredSchemas(): void { } /** - * Characterisation: a delete check that cannot read the cases, or has no id, lets the delete through. + * A delete check that cannot read the cases blocks the delete as unverified (decision 224). + * + * Controls: without an id there is nothing to check, and a read that answers no cases passes. */ - public function testValidateDeletionPassesWhenCasesCannotBeRead(): void { + public function testValidateDeletionBlocksWhenCasesCannotBeRead(): void { $default = ['blocked' => false, 'requiresConfirmation' => false, 'message' => '']; $os = $this->scriptedObjectService(static function (): array { throw new \RuntimeException('down'); }); $this->svc->setContext($os, []); - $this->assertSame($default, $this->svc->validateDeletion('reg', 'ct-1')); + $this->assertSame( + [ + 'blocked' => true, + 'requiresConfirmation' => false, + 'unverified' => true, + 'message' => 'Cannot delete case type: the check for cases that use this type could not be completed. ' + . 'Nothing was deleted. Try again later.', + ], + $this->svc->validateDeletion('reg', 'ct-1') + ); $this->assertSame($default, $this->svc->validateDeletion('reg', '')); + + $this->svc->setContext($this->scriptedObjectService(static fn (): array => []), []); + $this->assertSame($default, $this->svc->validateDeletion('reg', 'ct-1')); } /** * Characterisation: a case counts as closed only when its status is a final status type that - * could be read; no status, another status, or an unreadable final list all count as active. + * could be read; no status or another status counts as active. An unreadable final list is unverified. */ public function testValidateDeletionCountsActiveAndClosedCases(): void { $cases = [['status' => 'final'], ['status' => 'open'], ['status' => ''], [], ['status' => 'final-2']]; @@ -363,7 +381,8 @@ public function testValidateDeletionCountsActiveAndClosedCases(): void { throw new \RuntimeException('down'); }; $this->svc->setContext($this->scriptedObjectService($unreadable), []); - $this->assertStringContainsString('2 active case(s)', $this->svc->validateDeletion('reg', 'ct-1')['message']); + // Final statuses that cannot be read no longer count every case as active: the check did not run. + $this->assertTrue($this->svc->validateDeletion('reg', 'ct-1')['unverified']); $closedOnly = static function (string $register, string $schema, array $filters): array { if ($schema === 'case') { From 3f7560afeb77e1376b3833da8f3a16cd014e1849 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 12 Oct 2026 18:59:55 +0200 Subject: [PATCH 2/8] docs(openspec): design for the StatusTransitionService split into two facades method-decomposition slice 8h. The engine couples to 18 classes against 13 and takes 14 constructor arguments. The design gives it one facade over what a case declares and one over what a move writes; the engine keeps the order of the checks. --- .../changes/method-decomposition/design.md | 85 +++++++++++++++++++ .../changes/method-decomposition/tasks.md | 10 +++ 2 files changed, 95 insertions(+) diff --git a/openspec/changes/method-decomposition/design.md b/openspec/changes/method-decomposition/design.md index 8a4af093a5..52a9777130 100644 --- a/openspec/changes/method-decomposition/design.md +++ b/openspec/changes/method-decomposition/design.md @@ -68,6 +68,91 @@ Reduce constructor parameters by grouping related dependencies into composite se - **`lib/Service/MappingTransformService.php`** — Mapping data transformation (extracted from ZgwMappingService) - **`lib/Settings/zgw_mappings/*.json`** — Mapping configuration files (zaak.json, documents.json, besluit.json, catalogi.json) +## StatusTransitionService: two facades (slice 8h) + +Measured on the stack head (12 Oct): `StatusTransitionService` couples to 18 classes against a limit of 13 and takes 14 constructor arguments against 10. It carries two suppressions for it, `CouplingBetweenObjects` and `ExcessiveParameterList`. Its complexity is not the problem: the earlier split into `Transitions/*` already took the work out. What is left is an engine that holds every one of those classes itself. + +A move is not a design here. Folding two collaborators into a third to lower the count would put a concern back where it was split from, which is what the suppression says. The engine asks two kinds of question, so it gets two collaborators. + +### What the engine asks + +| The engine asks | Today it holds | After | +|---|---|---| +| What does this case declare about its status and its moves? | `StatusDeclarations`, `TransitionDeclarations`, `OfferedTransitions`, `StatusChecklist` | `Transitions/CaseDeclarations` | +| What does a move write, and what did earlier moves write? | `CaseStatusStore`, `CaseResultWriter`, `SideEffectDispatcher` | `Transitions/CaseMoveLedger` | +| May this person take this move? | `TransitionAuthorizer`, `GuardRegistry`, `TransitionSpecReader`, `ProcessOwnedStatusRule` | unchanged, the engine keeps them | +| Which workflow does the case run on, and who is asking? | `WorkflowTemplateLoader`, `IUserSession`, `LoggerInterface` | unchanged | + +The engine keeps the decision: which transition, in which order the checks run, what is refused and what the caller is told. That order is the specification (REQ-STE-5-002, REQ-STE-12) and stays readable in one method. + +### `Transitions/CaseDeclarations` (the read side) + +One class over the four that read what an administrator declared. No write to the store goes through it. + +| Method | Takes over | Answers | +|---|---|---| +| `panel(array $case): array` | `StatusDeclarations::panelFor` | `waitingOn`, `dwell`, `derivation` of the status the case is in | +| `offered(array $transitions, array $case, string $currentId, string $userId): array` | `OfferedTransitions::from` | `transitions` and `withheld` | +| `assertOpen(array $transition, array $case, string $caseId, string $userId): void` | `TransitionDeclarations::withheldReasons`, `fourEyesRefusal`, `refusalSentence` and the two refusals the engine throws on them | throws `RefusedException` `transition-dependency-open` or `transition-four-eyes`, in that order | +| `stamp(array $case, string $toStatus): array` | `StatusDeclarations::applyStatusChange` | the case payload with the dwell bookkeeping of the status it enters | +| `retime(string $caseId, string $toStatus): void` | `StatusDeclarations::retime` | nothing; stops and starts the status timers | +| `checklistActions(string $statusTypeId, array $case, string $actor): array` | `StatusChecklist::actionsFor` | the actions the entered status brings | + +`stamp()` and `retime()` are on the read side on purpose. They change nothing in the store themselves: `stamp()` returns a payload the ledger saves, and `retime()` talks to the term engine. They belong with the class that knows what a status declares. + +### `Transitions/CaseMoveLedger` (the write side) + +One class over the three that touch the store or the outside world. It takes whole steps from the engine, not single calls, so it is not a pass-through. + +| Method | Takes over | Notes | +|---|---|---| +| `load(string $caseId): ?array` | `CaseStatusStore::loadCase` | | +| `statusFacts(string $statusTypeId): array` | `lookupStatusName`, `lookupStatusColour`, `lookupStatusDescription` | one call, keys `statusName`, `statusColour`, `statusDescription` | +| `reloadUnchanged(string $caseId, int $readVersion, string $currentId): array` | the engine's `assertNoConcurrentChange()` | throws `case_not_found` or `RefusedException` `transition-conflict` | +| `saveMove(array $case, string $caseId, string $toStatus, ?string $resultTypeId, int $readVersion): array` | the store half of `writeMove()` and all of `applyClosingResult()` | closing result, end date, archival future, status and `@self.version` in ONE save; returns the saved case and its version | +| `saveFreeForm(array $case, string $toStatus): array` | `assertStatusBelongsToCaseType` and the save of `executeFreeForm()` | no closing result, as today | +| `recordAndDispatch(array $case, array $move, array $actions): array` | `writeStatusRecord`, `SideEffectDispatcher::dispatch`, the engine's `persistDispatchedActions()` | `$move` is `fromStatus`, `toStatus`, `label`, `comment`, `actor`, `evaluatedGuards`, `noWorkflowTemplate`; returns the record and the dispatched rows | +| `history(string $caseId): ?array` | `findStatusRecords` | `replay()` keeps the sort | + +`saveMove()` takes the payload AFTER `CaseDeclarations::stamp()`. The engine calls `stamp()`, then `saveMove()`, then `retime()`: the same order as today, and the only place the two facades meet. + +### The engine after the split + +Constructor, nine arguments: `WorkflowTemplateLoader`, `GuardRegistry`, `TransitionAuthorizer`, `TransitionSpecReader`, `IUserSession`, `LoggerInterface`, `ProcessOwnedStatusRule`, `CaseDeclarations`, `CaseMoveLedger`. With `RefusedException`, `GuardFailedException` and `RuntimeException` it couples to 12 classes. Both suppressions go and `phpmd.suppressWarnings` in `.debt-baseline.json` goes down by 2. + +The public methods and what they return do not change: `getAvailableTransitions()`, `execute()`, `executeFreeForm()`, `replay()`, `isAdmin()`, `ADMIN_GROUP_ID`. The container builds both facades by type, so no registration changes. Every class that injects the engine (32 files under `lib/`) is untouched. + +The seven classes behind the facades keep their own constructors and their own tests. Other callers of them, such as `Lifecycle/CaseActionProvider` and `CaseDocumentGenerationService` on `CaseStatusStore`, keep calling them directly. + +### What can go wrong + +- **The order of the refusals.** `assertTransitionAllowed()` refuses in a fixed order: from-status, open dependency, four eyes, group, guards. A caller sees the first. `assertOpen()` holds the second and third and is called between the first and the fourth. A characterisation test with a transition that fails all five pins the order before the split. +- **One save, not two.** The closing result, the dwell bookkeeping and the status land in one save. `saveMove()` must not save before the stamp is on the payload. Pinned by asserting the single `saveCase` payload. +- **The timer after the save.** `retime()` runs after `saveMove()` returns and not when it throws. +- **Coverage metadata.** Every test that reaches the engine executes the two new classes. Each needs both in `@uses`, or CI marks it risky. No coverage driver runs locally by default: use the pcov image (`d179-logs/pcov.sh`) on `tests/Unit` before the push. +- **Whole-tree PHPMD only.** Coupling does not show in a per-file run. Measure with a fresh cache (`HOME` and `TMPDIR` on empty directories). + +### Characterisation first + +Five test classes already cover the engine through the real collaborators: the group gate, the closing result, failed actions, replay, and the route a case runs on. Before the split, in its own commit and green on the single class, one more (`StatusTransitionServiceOrderTest`) pins what none of them does: + +1. the order of the five refusals, and that nothing is saved on any of them; +2. `transition-conflict` when the version or the status changed between read and save; +3. `case_not_found`, `transition_not_found` and `transition_missing_to_status`; +4. the payload of the one save of a closing move, with the stamp and the version on it; +5. `retime()` after the save and not after a refused save; +6. a status record whose dispatched actions cannot be written back: logged at error, the move still answers; +7. the free-form refusals (not an administrator, a status the process owns, a status of another case type); +8. the shape of `getAvailableTransitions()` without a case, without a workflow, and with one: `withheld` always present. + +The test builds the engine through one helper, so the constructor change in the next commit touches that helper and the six existing constructions only. + +### Steps + +1. This design. +2. Characterisation test, green on the single class (own commit). +3. `CaseDeclarations` and `CaseMoveLedger` with their own unit tests; the engine delegates; both suppressions removed; baseline lowered by 2. The characterisation test and the five existing ones stay green without an edit beyond the constructor helper. + ## Implementation Order 1. **Phase 1 (Priority 1 controllers):** ZrcController, ZtcController, BrcController, DrcController diff --git a/openspec/changes/method-decomposition/tasks.md b/openspec/changes/method-decomposition/tasks.md index cfb1c1f902..76d1d54f4a 100644 --- a/openspec/changes/method-decomposition/tasks.md +++ b/openspec/changes/method-decomposition/tasks.md @@ -493,3 +493,13 @@ characterisation test with a mapped ObjectService is a sufficient safety net; th active. Behaviour kept, not changed here. Own tests: `tests/Unit/Service/Zgw/ZtcConceptRulesTest` (6), `ZtcTypeReferencesTest` (4, every search asserted with its schema) and `ZtcPublishAndDeleteChecksTest` (6). + - [ ] 8h `StatusTransitionService` (2: CouplingBetweenObjects 18 against 13, and + ExcessiveParameterList, 14 constructor arguments against 10). Not a move: the design is in + design.md, "StatusTransitionService: two facades". `Transitions/CaseDeclarations` over the + four classes that read what was declared, `Transitions/CaseMoveLedger` over the three + that write. The engine keeps the decision and the order of the checks. + - [x] Design written (design.md). + - [ ] Characterisation: `tests/Unit/Service/StatusTransitionServiceOrderTest.php`, the eight + points of the design, green on the single class, own commit. + - [ ] The two facades with their own tests, the engine delegating, both suppressions removed, + `phpmd.suppressWarnings` lowered by 2. From 96fe20b8489a2cb1e50f3eda6a8d9e9043926581 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 12 Oct 2026 19:02:42 +0200 Subject: [PATCH 3/8] test(transitions): pin the order of the engine's checks and writes Characterisation before the StatusTransitionService split (method-decomposition slice 8h), green on the single class: the order of the five refusals, the conflict check, the one save of a closing move with the version it read, the timer after the save, the record that cannot be written back, the free-form refusals and the shape of the available transitions. Seven mutations of the engine turn it red. --- .../StatusTransitionServiceOrderTest.php | 568 ++++++++++++++++++ 1 file changed, 568 insertions(+) create mode 100644 tests/Unit/Service/StatusTransitionServiceOrderTest.php diff --git a/tests/Unit/Service/StatusTransitionServiceOrderTest.php b/tests/Unit/Service/StatusTransitionServiceOrderTest.php new file mode 100644 index 0000000000..3c6ec4ed6a --- /dev/null +++ b/tests/Unit/Service/StatusTransitionServiceOrderTest.php @@ -0,0 +1,568 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Dossiq\Tests\Unit\Service; + +use OCA\Dossiq\Exception\RefusedException; +use OCA\Dossiq\Service\Lifecycle\ProcessOwnedStatusRule; +use OCA\Dossiq\Service\Status\StatusDeclarations; +use OCA\Dossiq\Service\StatusTransitionService; +use OCA\Dossiq\Service\Transitions\CaseResultWriter; +use OCA\Dossiq\Service\Transitions\CaseStatusStore; +use OCA\Dossiq\Service\Transitions\GuardFailedException; +use OCA\Dossiq\Service\Transitions\GuardRegistry; +use OCA\Dossiq\Service\Transitions\OfferedTransitions; +use OCA\Dossiq\Service\Transitions\SideEffectDispatcher; +use OCA\Dossiq\Service\Transitions\StatusChecklist; +use OCA\Dossiq\Service\Transitions\TransitionAuthorizer; +use OCA\Dossiq\Service\Transitions\TransitionDeclarations; +use OCA\Dossiq\Service\Transitions\TransitionSpecReader; +use OCA\Dossiq\Service\WorkflowTemplateLoader; +use OCP\IUserSession; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; +use RuntimeException; + +/** + * The order of the engine's checks and writes. + * + * @covers \OCA\Dossiq\Service\StatusTransitionService + * @uses \OCA\Dossiq\Exception\RefusedException + * @uses \OCA\Dossiq\Service\Transitions\GuardFailedException + * @uses \OCA\Dossiq\Service\Transitions\TransitionSpecReader + * + * @spec openspec/changes/method-decomposition/tasks.md + */ +class StatusTransitionServiceOrderTest extends TestCase { + private const TRANSITION = ['id' => 't1', 'label' => 'Besluiten', 'fromStatus' => 'st-open', 'toStatus' => 'st-closed']; + + /** + * What the collaborators answer. A test changes an entry before it builds the engine. + * + * @var array + */ + private array $world = []; + + /** + * Every write and side effect, in the order it happened. + * + * @var array + */ + private array $calls = []; + + /** + * The errors the engine logged. + * + * @var array + */ + private array $errors = []; + + /** + * A world in which the move t1 goes through: an open case at version 3, nothing withheld. + * + * @return void + */ + protected function setUp(): void { + $case = ['id' => 'case-1', 'caseType' => 'ct-1', 'status' => 'st-open', '@self' => ['version' => 3]]; + $this->world = [ + 'cases' => [$case, $case], + 'transition' => self::TRANSITION, + 'template' => ['transitions' => [self::TRANSITION]], + 'withheld' => [], + 'fourEyes' => null, + 'groupOk' => true, + 'admin' => true, + 'guards' => [['type' => 'role', 'passed' => true]], + 'final' => false, + 'saveThrows' => null, + 'updateThrows' => null, + 'handSetRefused' => null, + 'foreignStatus' => null, + ]; + }//end setUp() + + /** + * Build the engine over the world, every collaborator a double that records its writes. + * + * The one place the constructor is spelled out, so the split touches this helper and no test. + * + * @return StatusTransitionService + */ + private function engine(): StatusTransitionService { + $world = &$this->world; + + $loader = $this->createMock(WorkflowTemplateLoader::class); + $loader->method('getTransitionForCase')->willReturnCallback(static fn (): ?array => $world['transition']); + $loader->method('getTemplateForCase')->willReturnCallback(static fn (): ?array => $world['template']); + + $store = $this->createMock(CaseStatusStore::class); + $store->method('loadCase')->willReturnCallback( + static function () use (&$world): ?array { + return array_shift($world['cases']); + } + ); + $store->method('lookupStatusName')->willReturnCallback(static fn (string $statusTypeId): string => 'name of '.$statusTypeId); + $store->method('lookupStatusColour')->willReturn('#123456'); + $store->method('lookupStatusDescription')->willReturn('What it means.'); + $store->method('saveCase')->willReturnCallback( + function (array $case) use (&$world): array { + if ($world['saveThrows'] !== null) { + throw $world['saveThrows']; + } + + $this->calls[] = ['saveCase', $case]; + $case['@self']['version'] = (($case['@self']['version'] ?? 0) + 1); + + return $case; + } + ); + $store->method('writeStatusRecord')->willReturnCallback( + function (...$arguments): array { + $names = ['caseId', 'toStatus', 'fromStatus', 'label', 'comment', 'evaluatedGuards', 'noWorkflowTemplate', 'actor']; + $this->calls[] = ['writeStatusRecord', array_combine($names, $arguments)]; + return ['id' => 'rec-1']; + } + ); + $store->method('updateStatusRecord')->willReturnCallback( + function (array $record) use (&$world): array { + if ($world['updateThrows'] !== null) { + throw $world['updateThrows']; + } + + $this->calls[] = ['updateStatusRecord', $record]; + + return ($record + ['updated' => true]); + } + ); + $store->method('assertStatusBelongsToCaseType')->willReturnCallback( + static function () use (&$world): void { + if ($world['foreignStatus'] !== null) { + throw $world['foreignStatus']; + } + } + ); + + $statuses = $this->createMock(StatusDeclarations::class); + $statuses->method('panelFor')->willReturn(['waitingOn' => 'us', 'dwell' => ['days' => 2], 'derivation' => null]); + $statuses->method('applyStatusChange')->willReturnCallback( + static fn (array $case, string $toStatus): array => ($case + ['statusEnteredAt' => 'stamped for '.$toStatus]) + ); + $statuses->method('retime')->willReturnCallback( + function (string $caseId, string $toStatus): ?string { + $this->calls[] = ['retime', $caseId, $toStatus]; + return null; + } + ); + + $moves = $this->createMock(TransitionDeclarations::class); + $moves->method('withheldReasons')->willReturnCallback(static fn (): array => $world['withheld']); + $moves->method('fourEyesRefusal')->willReturnCallback(static fn (): ?array => $world['fourEyes']); + $moves->method('refusalSentence')->willReturn('Somebody else has to take this move.'); + + $offered = $this->createMock(OfferedTransitions::class); + $offered->method('from')->willReturn(['transitions' => [['id' => 't1']], 'withheld' => [['id' => 't2', 'reasons' => ['advice']]]]); + + $authorizer = $this->createMock(TransitionAuthorizer::class); + $authorizer->method('isTransitionGroupAuthorized')->willReturnCallback(static fn (): bool => $world['groupOk']); + $authorizer->method('isAdmin')->willReturnCallback(static fn (): bool => $world['admin']); + + $guards = $this->createMock(GuardRegistry::class); + $guards->method('evaluateAll')->willReturnCallback(static fn (): array => $world['guards']); + $guards->method('knows')->willReturn(false); + + $results = $this->createMock(CaseResultWriter::class); + $results->method('isFinalStatus')->willReturnCallback(static fn (): bool => $world['final']); + $results->method('resolveClosingResult')->willReturn('result-1'); + $results->method('archivalFuture')->willReturn(['archiveNomination' => 'vernietigen']); + + $checklist = $this->createMock(StatusChecklist::class); + $checklist->method('actionsFor')->willReturn([['type' => 'createTask', 'title' => 'Dossier controleren']]); + + $dispatcher = $this->createMock(SideEffectDispatcher::class); + $dispatcher->method('dispatch')->willReturnCallback( + function (array $actions, array $case, array $transitionContext): array { + $this->calls[] = ['dispatch', $actions, $transitionContext]; + return [['type' => 'createTask', 'ok' => true]]; + } + ); + + $handSet = $this->createMock(ProcessOwnedStatusRule::class); + $handSet->method('requireHandSetAllowed')->willReturnCallback( + static function () use (&$world): void { + if ($world['handSetRefused'] !== null) { + throw $world['handSetRefused']; + } + } + ); + + $logger = $this->createMock(LoggerInterface::class); + $logger->method('error')->willReturnCallback( + function (string $message): void { + $this->errors[] = $message; + } + ); + + return new StatusTransitionService( + templateLoader: $loader, + guardRegistry: $guards, + sideEffectDispatcher: $dispatcher, + store: $store, + authorizer: $authorizer, + specReader: new TransitionSpecReader(), + userSession: $this->createMock(IUserSession::class), + logger: $logger, + resultWriter: $results, + statusChecklist: $checklist, + declarations: $statuses, + declaredMoves: $moves, + offered: $offered, + processOwnedStatus: $handSet, + ); + }//end engine() + + /** + * The rule a refused move names, or the message of another failure. + * + * @param \Closure $move The call on the engine. + * + * @return string + */ + private function refusalOf(\Closure $move): string { + try { + $move(); + } catch (RefusedException $refused) { + return $refused->getRule().' '.$refused->getStatus(); + } catch (RuntimeException $failure) { + return $failure->getMessage(); + } + + return 'went through'; + }//end refusalOf() + + /** + * The names of the calls made, in order. + * + * @return array + */ + private function callNames(): array { + return array_column($this->calls, 0); + }//end callNames() + + /** + * A move with everything against it is refused for the first reason in a fixed order: the status + * it starts from, an open dependency, four eyes, the group, the guards. Nothing is written on any. + * + * Control: with nothing against it the same move goes through. + * + * @return void + */ + public function testTheRefusalsComeInAFixedOrder(): void { + $case = ['id' => 'case-1', 'caseType' => 'ct-1', 'status' => 'st-open', '@self' => ['version' => 3]]; + $fixes = [ + 'transition' => self::TRANSITION, + 'withheld' => [], + 'fourEyes' => null, + 'groupOk' => true, + 'guards' => [['type' => 'role', 'passed' => true]], + ]; + $this->world = array_merge( + $this->world, + [ + 'transition' => (['fromStatus' => 'st-other'] + self::TRANSITION), + 'withheld' => ['the advice of the fire brigade', 'the fee'], + 'fourEyes' => ['reason' => 'same-person'], + 'groupOk' => false, + 'guards' => [['type' => 'role', 'passed' => false]], + ] + ); + + $seen = []; + foreach ($fixes as $key => $fixed) { + $this->world['cases'] = [$case, $case]; + $seen[] = $this->refusalOf(fn () => $this->engine()->execute('case-1', 't1', null, 'jan')); + $this->world[$key] = $fixed; + } + + $this->assertSame( + [ + 'transition-from-status-mismatch 409', + 'transition-dependency-open 409', + 'transition-four-eyes 403', + 'transition-unauthorized 403', + 'guard_failed', + ], + $seen + ); + $this->assertSame([], $this->calls, 'a refused move writes nothing'); + + $this->world['cases'] = [$case, $case]; + $this->assertSame('went through', $this->refusalOf(fn () => $this->engine()->execute('case-1', 't1', null, 'jan'))); + }//end testTheRefusalsComeInAFixedOrder() + + /** + * The open dependency is named in the sentence, the first one only, and failed guards travel with + * their exception. + * + * @return void + */ + public function testARefusalSaysWhatItWaitsOn(): void { + $this->world['withheld'] = ['the advice of the fire brigade', 'the fee']; + try { + $this->engine()->execute('case-1', 't1', null, 'jan'); + $this->fail('an open dependency must refuse'); + } catch (RefusedException $refused) { + $this->assertSame('This move is waiting on the advice of the fire brigade.', $refused->getSentence()); + } + + $this->setUp(); + $this->world['guards'] = [['type' => 'role', 'passed' => true], ['type' => 'field', 'passed' => false]]; + try { + $this->engine()->execute('case-1', 't1', null, 'jan'); + $this->fail('a failed guard must refuse'); + } catch (GuardFailedException $failed) { + $this->assertSame([['type' => 'field', 'passed' => false]], $failed->getFailedGuards()); + } + }//end testARefusalSaysWhatItWaitsOn() + + /** + * A case, a transition or a target status that is not there stops the move by name. + * + * @return void + */ + public function testWhatIsMissingStopsTheMove(): void { + $this->world['cases'] = []; + $noCase = $this->refusalOf(fn () => $this->engine()->execute('case-1', 't1', null, 'jan')); + + $this->setUp(); + $this->world['transition'] = null; + $noTransition = $this->refusalOf(fn () => $this->engine()->execute('case-1', 't1', null, 'jan')); + + $this->setUp(); + $this->world['transition'] = ['id' => 't1', 'fromStatus' => 'st-open']; + $noTarget = $this->refusalOf(fn () => $this->engine()->execute('case-1', 't1', null, 'jan')); + + $this->assertSame(['case_not_found', 'transition_not_found', 'transition_missing_to_status'], [$noCase, $noTransition, $noTarget]); + $this->assertSame([], $this->calls); + }//end testWhatIsMissingStopsTheMove() + + /** + * A case that changed between the read and the save is a conflict: another version, another + * status, or gone. Nothing is written. + * + * @return void + */ + public function testAChangeBetweenReadAndSaveIsAConflict(): void { + $case = $this->world['cases'][0]; + + $this->world['cases'] = [$case, (['@self' => ['version' => 4]] + $case)]; + $version = $this->refusalOf(fn () => $this->engine()->execute('case-1', 't1', null, 'jan')); + + $this->world['cases'] = [$case, (['status' => 'st-elsewhere'] + $case)]; + $status = $this->refusalOf(fn () => $this->engine()->execute('case-1', 't1', null, 'jan')); + + $this->world['cases'] = [$case]; + $gone = $this->refusalOf(fn () => $this->engine()->execute('case-1', 't1', null, 'jan')); + + $this->assertSame(['transition-conflict 409', 'transition-conflict 409', 'case_not_found'], [$version, $status, $gone]); + $this->assertSame([], $this->calls); + }//end testAChangeBetweenReadAndSaveIsAConflict() + + /** + * A closing move writes its result, end date, archival future, stamp, status and the version it + * read in ONE save, then retimes, records, dispatches the checklist and writes the outcome back. + * + * @return void + */ + public function testAClosingMoveIsOneSaveThenTheRest(): void { + $this->world['final'] = true; + $this->world['cases'][1] += ['endDate' => '2026-10-01T09:00:00+02:00']; + + $answer = $this->engine()->execute('case-1', 't1', 'Klaar.', 'jan', 'rt-1'); + + $this->assertSame(['saveCase', 'retime', 'writeStatusRecord', 'dispatch', 'updateStatusRecord'], $this->callNames()); + $this->assertSame( + [ + 'id' => 'case-1', + 'caseType' => 'ct-1', + 'status' => 'st-closed', + '@self' => ['version' => 3], + 'endDate' => '2026-10-01', + 'result' => 'result-1', + 'archiveNomination' => 'vernietigen', + 'statusEnteredAt' => 'stamped for st-closed', + ], + $this->calls[0][1] + ); + $this->assertSame(['retime', 'case-1', 'st-closed'], $this->calls[1]); + $this->assertSame( + [ + 'caseId' => 'case-1', + 'toStatus' => 'st-closed', + 'fromStatus' => 'st-open', + 'label' => 'Besluiten', + 'comment' => 'Klaar.', + 'evaluatedGuards' => [['type' => 'role', 'passed' => true]], + 'noWorkflowTemplate' => false, + 'actor' => 'jan', + ], + $this->calls[2][1] + ); + $this->assertSame([['type' => 'createTask', 'title' => 'Dossier controleren']], $this->calls[3][1]); + $this->assertSame( + ['fromStatus' => 'st-open', 'toStatus' => 'st-closed', 'transitionLabel' => 'Besluiten', 'userId' => 'jan', 'statusRecordUuid' => 'rec-1'], + $this->calls[3][2] + ); + $this->assertSame( + [ + 'status' => 'ok', + 'statusRecord' => ['id' => 'rec-1', 'dispatchedActions' => [['type' => 'createTask', 'ok' => true]], 'updated' => true], + 'dispatchedActions' => [['type' => 'createTask', 'ok' => true]], + 'failedActions' => [], + 'version' => 4, + ], + $answer + ); + }//end testAClosingMoveIsOneSaveThenTheRest() + + /** + * The version the engine read travels with the save, also for a case that carries it at the top + * level and has no `@self` block. + * + * @return void + */ + public function testTheVersionReadTravelsWithTheSave(): void { + $case = ['id' => 'case-1', 'caseType' => 'ct-1', 'status' => 'st-open', 'version' => 7]; + $this->world['cases'] = [$case, $case]; + + $this->engine()->execute('case-1', 't1', null, 'jan'); + + $this->assertSame(['version' => 7], $this->calls[0][1]['@self']); + }//end testTheVersionReadTravelsWithTheSave() + + /** + * A save that fails stops the move there: no timer, no record, no side effect. + * + * @return void + */ + public function testNothingFollowsASaveThatFails(): void { + $this->world['saveThrows'] = new RuntimeException('store down'); + + $this->assertSame('store down', $this->refusalOf(fn () => $this->engine()->execute('case-1', 't1', null, 'jan'))); + $this->assertSame([], $this->calls); + }//end testNothingFollowsASaveThatFails() + + /** + * A record whose dispatched actions cannot be written back is logged at error; the move answers + * with the record as it was written first. + * + * @return void + */ + public function testARecordThatCannotBeUpdatedIsLoggedAndTheMoveAnswers(): void { + $this->world['updateThrows'] = new RuntimeException('store down'); + + $answer = $this->engine()->execute('case-1', 't1', null, 'jan'); + + $this->assertSame('ok', $answer['status']); + $this->assertSame(['id' => 'rec-1', 'dispatchedActions' => [['type' => 'createTask', 'ok' => true]]], $answer['statusRecord']); + $this->assertSame(['StatusTransitionService: dispatchedActions persist failed'], $this->errors); + }//end testARecordThatCannotBeUpdatedIsLoggedAndTheMoveAnswers() + + /** + * A free-form move is refused to anyone but an administrator, where the process owns the status, + * and for a status of another case type, in that order and before any write. + * + * Control: an administrator on a hand-set case type moves the case, stamped, without a closing result. + * + * @return void + */ + public function testTheFreeFormRefusalsAndTheFreeFormMove(): void { + $case = $this->world['cases'][0]; + $this->world['cases'] = array_fill(0, 4, $case); + $this->world['admin'] = false; + $this->world['handSetRefused'] = new RuntimeException('process_owned'); + $this->world['foreignStatus'] = new RuntimeException('status_not_of_case_type'); + + $seen = []; + foreach (['admin' => true, 'handSetRefused' => null, 'foreignStatus' => null] as $key => $fixed) { + $seen[] = $this->refusalOf(fn () => $this->engine()->executeFreeForm('case-1', 'st-elsewhere', null, 'jan')); + $this->world[$key] = $fixed; + } + + $this->assertSame(['forbidden_admin_only', 'process_owned', 'status_not_of_case_type'], $seen); + $this->assertSame([], $this->calls); + + $this->world['final'] = true; + $answer = $this->engine()->executeFreeForm('case-1', 'st-elsewhere', 'Hersteld.', 'jan'); + + $this->assertSame(['saveCase', 'retime', 'writeStatusRecord', 'dispatch'], $this->callNames()); + $this->assertSame('st-elsewhere', $this->calls[0][1]['status']); + $this->assertSame('stamped for st-elsewhere', $this->calls[0][1]['statusEnteredAt']); + $this->assertArrayNotHasKey('result', $this->calls[0][1], 'a free-form move records no closing result'); + $this->assertSame('Free-form transition', $this->calls[2][1]['label']); + $this->assertTrue($this->calls[2][1]['noWorkflowTemplate']); + $this->assertSame(['status', 'statusRecord', 'dispatchedActions', 'failedActions'], array_keys($answer)); + }//end testTheFreeFormRefusalsAndTheFreeFormMove() + + /** + * The available transitions: empty without a case; the status facts and an empty `withheld` + * without a workflow; the offered and the withheld moves with one. + * + * @return void + */ + public function testTheShapeOfTheAvailableTransitions(): void { + $current = [ + 'statusId' => 'st-open', + 'statusName' => 'name of st-open', + 'statusColour' => '#123456', + 'statusDescription' => 'What it means.', + 'waitingOn' => 'us', + 'dwell' => ['days' => 2], + ]; + + $with = $this->engine()->getAvailableTransitions('case-1', 'jan'); + + $this->setUp(); + $this->world['template'] = null; + $without = $this->engine()->getAvailableTransitions('case-1', 'jan'); + + $this->setUp(); + $this->world['template'] = ['transitions' => 'not a list']; + $broken = $this->engine()->getAvailableTransitions('case-1', 'jan'); + + $this->setUp(); + $this->world['cases'] = []; + $noCase = $this->engine()->getAvailableTransitions('case-1', 'jan'); + + $this->assertSame( + ['transitions' => [['id' => 't1']], 'withheld' => [['id' => 't2', 'reasons' => ['advice']]], 'current' => $current, 'derivation' => null], + $with + ); + $this->assertSame(['transitions' => [], 'withheld' => [], 'current' => $current, 'derivation' => null], $without); + $this->assertSame($without, $broken); + $this->assertSame(['transitions' => [], 'current' => []], $noCase); + }//end testTheShapeOfTheAvailableTransitions() +}//end class From a3a1c0a7b08ce0dd4778c04e1bfdc53030cce9cc Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 12 Oct 2026 19:59:38 +0200 Subject: [PATCH 4/8] refactor(transitions): the status engine asks two facades instead of fourteen collaborators method-decomposition slice 8h. StatusTransitionService coupled to 18 classes against 13 and took 14 constructor arguments. Transitions/CaseDeclarations reads what a case declares about its status and its moves, and Transitions/CaseMoveLedger holds what a move writes. The engine keeps the decision and the order of the checks, takes nine arguments and couples to 12 classes. Both suppressions are gone; phpmd.suppressWarnings 183 to 181. The public methods and their answers are unchanged. The characterisation test and the five existing engine tests are green with only their constructor call changed. --- .debt-baseline.json | 2 +- lib/Service/StatusTransitionService.php | 371 +++--------------- lib/Service/Transitions/CaseDeclarations.php | 187 +++++++++ lib/Service/Transitions/CaseMoveLedger.php | 291 ++++++++++++++ .../changes/method-decomposition/design.md | 21 +- .../changes/method-decomposition/tasks.md | 26 +- .../Unit/Lifecycle/CaseActionProviderTest.php | 30 +- ...StatusTransitionServiceEnforcementTest.php | 30 +- ...atusTransitionServiceFailedActionsTest.php | 30 +- .../StatusTransitionServiceOrderTest.php | 24 +- ...sTransitionServiceReplayRegressionTest.php | 42 +- .../StatusTransitionServiceResultTest.php | 30 +- .../StatusTransitionServiceRouteSeamTest.php | 42 +- .../Transitions/CaseDeclarationsTest.php | 174 ++++++++ .../Transitions/CaseMoveLedgerTest.php | 293 ++++++++++++++ 15 files changed, 1186 insertions(+), 407 deletions(-) create mode 100644 lib/Service/Transitions/CaseDeclarations.php create mode 100644 lib/Service/Transitions/CaseMoveLedger.php create mode 100644 tests/Unit/Service/Transitions/CaseDeclarationsTest.php create mode 100644 tests/Unit/Service/Transitions/CaseMoveLedgerTest.php diff --git a/.debt-baseline.json b/.debt-baseline.json index b2ac3b560c..42685da2d9 100644 --- a/.debt-baseline.json +++ b/.debt-baseline.json @@ -2,7 +2,7 @@ "eslint.inlineDisables": 14, "eslint.suppressedViolations": 308, "phpcs.inlineIgnores": 3, - "phpmd.suppressWarnings": 183, + "phpmd.suppressWarnings": 181, "phpstan.ignorePatterns": 11, "phpstan.inlineIgnores": 4, "phpunit.skips": 6, diff --git a/lib/Service/StatusTransitionService.php b/lib/Service/StatusTransitionService.php index 0bf5bd3f9c..61ac6b95ec 100644 --- a/lib/Service/StatusTransitionService.php +++ b/lib/Service/StatusTransitionService.php @@ -13,10 +13,13 @@ * - dispatch automatic actions sequentially (via SideEffectDispatcher) * - replay transition history from the `statusRecord` chain * - * Three collaborators carry the concerns that are not transition decisions: - * {@see Transitions\CaseStatusStore} owns every OpenRegister read/write, - * {@see Transitions\TransitionAuthorizer} owns the OR-RBAC group gate, and - * {@see Transitions\TransitionSpecReader} owns the template dialects a + * The engine keeps the decision: which transition, in which order the checks + * run, what is refused and what the caller is told. It asks two collaborators + * for everything else. {@see Transitions\CaseDeclarations} reads what a case + * declares about its status and its moves. {@see Transitions\CaseMoveLedger} + * holds every read and write of the store, the closing result and the side + * effects. {@see Transitions\TransitionAuthorizer} owns the OR-RBAC group gate + * and {@see Transitions\TransitionSpecReader} the template dialects a * transition's guards and actions may be spelled in. * * Identity is ALWAYS derived from IUserSession when the caller does not pass @@ -44,16 +47,11 @@ use OCA\Dossiq\Exception\RefusedException; use OCA\Dossiq\Service\Lifecycle\ProcessOwnedStatusRule; -use OCA\Dossiq\Service\Status\StatusDeclarations; -use OCA\Dossiq\Service\Transitions\CaseResultWriter; -use OCA\Dossiq\Service\Transitions\CaseStatusStore; +use OCA\Dossiq\Service\Transitions\CaseDeclarations; +use OCA\Dossiq\Service\Transitions\CaseMoveLedger; use OCA\Dossiq\Service\Transitions\GuardFailedException; use OCA\Dossiq\Service\Transitions\GuardRegistry; -use OCA\Dossiq\Service\Transitions\SideEffectDispatcher; -use OCA\Dossiq\Service\Transitions\StatusChecklist; use OCA\Dossiq\Service\Transitions\TransitionAuthorizer; -use OCA\Dossiq\Service\Transitions\OfferedTransitions; -use OCA\Dossiq\Service\Transitions\TransitionDeclarations; use OCA\Dossiq\Service\Transitions\TransitionSpecReader; use OCP\IUserSession; use Psr\Log\LoggerInterface; @@ -62,14 +60,6 @@ /** * The status-transition engine. * - * @SuppressWarnings(PHPMD.CouplingBetweenObjects) — the collaborators ARE the - * decomposition: persistence, the group gate, the template dialects, the - * closing result and the status checklist each live in their own class, - * and folding any of them back in to satisfy the count would put the - * concern back where it was split from. - * @SuppressWarnings(PHPMD.ExcessiveParameterList) — same reason, counted at the - * constructor: every parameter is one of those collaborators, injected. - * * @spec openspec/changes/status-transition-engine/tasks.md#T10 */ class StatusTransitionService { @@ -89,34 +79,24 @@ class StatusTransitionService { * * @param WorkflowTemplateLoader $templateLoader Active workflowTemplate loader * @param GuardRegistry $guardRegistry Guard registry - * @param SideEffectDispatcher $sideEffectDispatcher Side-effect dispatcher - * @param CaseStatusStore $store OpenRegister persistence for the engine * @param TransitionAuthorizer $authorizer OR-RBAC group gate * @param TransitionSpecReader $specReader Guard/action shape reader * @param IUserSession $userSession Current session * @param LoggerInterface $logger Logger - * @param CaseResultWriter $resultWriter Closing-result reader/writer - * @param StatusChecklist $statusChecklist The checklist a status brings with it - * @param StatusDeclarations $declarations What a status declares about itself - * @param TransitionDeclarations $declaredMoves What a transition declares about itself - * @param OfferedTransitions $offered Which moves this case offers and which it withholds * @param ProcessOwnedStatusRule $processOwnedStatus Refuses a hand-set status where the case type gives it to the process + * @param CaseDeclarations $declarations What the case declares about its status and its moves + * @param CaseMoveLedger $ledger What a move writes, and what earlier moves wrote */ public function __construct( private readonly WorkflowTemplateLoader $templateLoader, private readonly GuardRegistry $guardRegistry, - private readonly SideEffectDispatcher $sideEffectDispatcher, - private readonly CaseStatusStore $store, private readonly TransitionAuthorizer $authorizer, private readonly TransitionSpecReader $specReader, private readonly IUserSession $userSession, private readonly LoggerInterface $logger, - private readonly CaseResultWriter $resultWriter, - private readonly StatusChecklist $statusChecklist, - private readonly StatusDeclarations $declarations, - private readonly TransitionDeclarations $declaredMoves, - private readonly OfferedTransitions $offered, private readonly ProcessOwnedStatusRule $processOwnedStatus, + private readonly CaseDeclarations $declarations, + private readonly CaseMoveLedger $ledger, ) { }//end __construct() @@ -132,7 +112,7 @@ public function __construct( */ public function getAvailableTransitions(string $caseId, ?string $userId = null): array { $userId = $this->resolveUserId(explicit: $userId); - $case = $this->store->loadCase(caseId: $caseId); + $case = $this->ledger->load(caseId: $caseId); if ($case === null) { return ['transitions' => [], 'current' => []]; } @@ -149,7 +129,7 @@ public function getAvailableTransitions(string $caseId, ?string $userId = null): // The handler asks both questions in the same breath — what can I do // here, and why is the thing I expected not on offer — so they are // answered in one round trip rather than two. - $declared = $this->declarations->panelFor(case: $case); + $declared = $this->declarations->panel(case: $case); $result = [ 'transitions' => [], @@ -157,17 +137,14 @@ public function getAvailableTransitions(string $caseId, ?string $userId = null): // to test for the key would read an absent list as "nothing is in // the way", which is the one wrong answer that looks right. 'withheld' => [], - 'current' => [ - 'statusId' => $currentId, - 'statusName' => $this->store->lookupStatusName(statusTypeId: $currentId), - 'statusColour' => $this->store->lookupStatusColour(statusTypeId: $currentId), - // What this status MEANS, written by an administrator and - // rendered on the case. Published beside the name because both - // come off the same row. - 'statusDescription' => $this->store->lookupStatusDescription(statusTypeId: $currentId), - 'waitingOn' => $declared['waitingOn'], - 'dwell' => $declared['dwell'], - ], + // The name, the colour and what this status MEANS, written by an + // administrator and rendered on the case: all three come off the + // same row. + 'current' => array_merge( + ['statusId' => $currentId], + $this->declarations->statusFacts(statusTypeId: $currentId), + ['waitingOn' => $declared['waitingOn'], 'dwell' => $declared['dwell']], + ), 'derivation' => $declared['derivation'], ]; @@ -184,7 +161,7 @@ public function getAvailableTransitions(string $caseId, ?string $userId = null): // decision about one transition asked five ways, and it lives in its // own class. This engine's job is what happens when a handler TAKES // one. - $sorted = $this->offered->from( + $sorted = $this->declarations->offered( transitions: $transitions, case: $case, currentId: $currentId, @@ -231,7 +208,7 @@ public function execute( ?string $resultTypeId = null, ): array { $userId = $this->resolveUserId(explicit: $userId); - $case = $this->store->loadCase(caseId: $caseId); + $case = $this->ledger->load(caseId: $caseId); if ($case === null) { throw new RuntimeException('case_not_found'); } @@ -263,7 +240,7 @@ public function execute( // H2: Optimistic concurrency guard — re-load the case immediately before writing // and abort if its status changed since we read it (concurrent transition executed // between our guard evaluation and our save). - $caseAtSave = $this->assertNoConcurrentChange( + $caseAtSave = $this->ledger->reloadUnchanged( caseId: $caseId, readVersion: $readVersion, currentId: $currentId, @@ -405,31 +382,16 @@ private function writeMove( ?string $resultTypeId, int $readVersion, ): array { - $caseAtSave = $this->applyClosingResult( + $caseAtSave = $this->ledger->close( case: $caseAtSave, caseId: $caseId, toStatus: $toStatus, resultTypeId: $resultTypeId, ); - $caseAtSave = $this->declarations->applyStatusChange(case: $caseAtSave, toStatus: $toStatus); - - $caseAtSave['status'] = $toStatus; - - // One branch, not two. `isset() === false || is_array() === false` is - // two decision points for one question, and this class sits on PHPMD's - // complexity ceiling: the compound spelling is what pushed it over. - // A missing key reads as null here, and null is not an array. - $self = ($caseAtSave['@self'] ?? null); - if (is_array($self) === false) { - $self = []; - } - - $self['version'] = $readVersion; - $caseAtSave['@self'] = $self; - $savedCase = $this->store->saveCase(case: $caseAtSave); + $caseAtSave = $this->declarations->stamp(case: $caseAtSave, toStatus: $toStatus); - return [$savedCase, (int)(($savedCase['@self']['version'] ?? ($savedCase['version'] ?? 0)))]; + return $this->ledger->save(case: $caseAtSave, toStatus: $toStatus, readVersion: $readVersion); }//end writeMove() /** @@ -461,117 +423,32 @@ private function recordAndDispatch( string $userId, array $evaluatedGuards, ): array { - $label = (string)($transition['label'] ?? ''); $toStatus = (string)($transition['toStatus'] ?? ''); - $record = $this->store->writeStatusRecord( - caseId: $caseId, - toStatus: $toStatus, - fromStatus: $currentId, - label: $label, - comment: $comment, - evaluatedGuards: $evaluatedGuards, - noWorkflowTemplate: false, - actor: $userId, - ); - - $statusRecordId = (string)($record['id'] ?? ''); - $context = [ + $move = [ 'fromStatus' => $currentId, 'toStatus' => $toStatus, - 'transitionLabel' => $label, - 'userId' => $userId, - 'statusRecordUuid' => $statusRecordId, + 'label' => (string)($transition['label'] ?? ''), + 'comment' => $comment, + 'actor' => $userId, + 'evaluatedGuards' => $evaluatedGuards, + 'noWorkflowTemplate' => false, ]; + $record = $this->ledger->record(caseId: $caseId, move: $move); // The status the case just entered brings its own work. The checklist // actions go FIRST, so the tasks a phase asks for exist before whatever - // the transition itself does with them — a notification that lists the + // the transition itself does with them: a notification that lists the // case's open tasks is otherwise sent one dispatch too early. $actions = array_merge( - $this->statusChecklist->actionsFor(statusTypeId: $toStatus, case: $case, actor: $userId), + $this->declarations->checklistActions(statusTypeId: $toStatus, case: $case, actor: $userId), $this->specReader->extractActions(transition: $transition), ); - $dispatched = $this->sideEffectDispatcher->dispatch(actions: $actions, case: $case, transitionContext: $context); + $dispatched = $this->ledger->dispatch(actions: $actions, case: $case, move: $move, record: $record); // Update the statusRecord with the actual dispatched-action results. - $record = $this->persistDispatchedActions( - record: $record, - dispatched: $dispatched, - statusRecordId: $statusRecordId, - ); - - return [$record, $dispatched]; + return [$this->ledger->writeBack(record: $record, dispatched: $dispatched), $dispatched]; }//end recordAndDispatch() - /** - * Write the result a closing transition records, onto the case payload. - * - * Returns the case unchanged when the target status is not final. When it - * is, a result type is REQUIRED: a case that closes with no result answers - * none of the questions an archivist, a citizen or a WOO request will ask - * of it later. - * - * A closing transition also settles the case's END DATE and its ARCHIVAL - * FUTURE, both in this same save. Neither used to happen here. `endDate` - * was written only by the ZGW path (zrc-007a), so a case closed at the desk - * read as still open to every consumer that asks the zaak whether it has an - * einddatum; and the zrc-021 nomination was derived only over the API, so - * the same case closed two ways ended in two different archival states. - * Doing both here, rather than in a job afterwards, is what keeps them - * true of every closed case rather than of most of them. - * - * @param array $case The case payload about to be saved - * @param string $caseId Case UUID - * @param string $toStatus The target statusType UUID - * @param string|null $resultTypeId The chosen resultType UUID, or null - * - * @return array The case payload, with `result`, `endDate` and - * the archival fields set when closing - * - * @throws RuntimeException When a closing transition carries no result type - * - * @spec openspec/specs/status-transition-engine/spec.md - */ - private function applyClosingResult( - array $case, - string $caseId, - string $toStatus, - ?string $resultTypeId, - ): array { - if ($this->resultWriter->isFinalStatus(statusTypeId: $toStatus) === false) { - return $case; - } - - // An end date the case already carries is left alone: a handler who - // backdated the close meant it, and zrc-021 derives from that date. - $endDate = substr((string)($case['endDate'] ?? ''), 0, 10); - if ($endDate === '') { - $endDate = date('Y-m-d'); - } - - $case['endDate'] = $endDate; - - $resultId = $this->resultWriter->resolveClosingResult( - caseId: $caseId, - caseTypeId: (string)($case['caseType'] ?? ''), - resultTypeId: $resultTypeId, - ); - if ($resultId === null) { - return $case; - } - - $case['result'] = $resultId; - - return array_merge( - $case, - $this->resultWriter->archivalFuture( - case: $case, - resultTypeId: (string)$resultTypeId, - endDate: $endDate, - ) - ); - }//end applyClosingResult() - /** * Re-evaluate every server-side precondition for a transition. * @@ -608,45 +485,8 @@ private function assertTransitionAllowed( // are: the list a browser holds was true when it was fetched, and the // advice request can have arrived since. Withholding is the courtesy; // this is the rule. - $withheld = $this->declaredMoves->withheldReasons(transition: $transition, case: $case); - if ($withheld !== []) { - throw new RefusedException( - rule: 'transition-dependency-open', - sentence: 'This move is waiting on ' . $withheld[0] . '.', - status: RefusedException::STATUS_REFUSED, - ); - } - - // 🔑 FOUR EYES IS A NEGATIVE RULE ABOUT AN ACT, NOT A ROLE. The same - // person legitimately approves other cases they did not prepare, so - // there is no role to withhold this from: the engine reads who - // performed the named earlier act ON THIS CASE and refuses them. - // It refuses rather than withholds, deliberately. A colleague opening - // the same case must see the move, and a list that hid it per reader - // would make two handlers disagree about what the case offers. - $refusal = $this->declaredMoves->fourEyesRefusal( - transition: $transition, - caseId: $caseId, - userId: $userId, - ); - if ($refusal !== null) { - throw new RefusedException( - rule: 'transition-four-eyes', - sentence: $this->declaredMoves->refusalSentence(refusal: $refusal), - status: RefusedException::STATUS_FORBIDDEN, - ); - } + $this->declarations->assertOpen(transition: $transition, case: $case, caseId: $caseId, userId: $userId); - // OR-RBAC role-routing gate (ADR-022). At publish time - // WorkflowDefinitionService resolves each transition's assignee role - // to its `roleType.ncGroupId` and freezes the literal group id(s) on - // the transition `authorization` list — the same OR PR #153 gate - // format OR enforces declaratively on schemas that carry an - // x-openregister-lifecycle. `case.status` is a per-caseType dynamic - // state machine with no static lifecycle table, so OR cannot enforce - // it on saveObject; this engine therefore enforces the SAME group - // model here using OR's single trusted membership check (IGroupManager), - // not a bespoke role-resolution scheme. An empty/absent list = open. if ($this->authorizer->isTransitionGroupAuthorized(transition: $transition, userId: $userId) === false) { throw new RefusedException( rule: 'transition-unauthorized', @@ -671,80 +511,6 @@ private function assertTransitionAllowed( return $eval; }//end assertTransitionAllowed() - /** - * Re-load the case immediately before writing and abort when another - * transition landed in the meantime (H2 optimistic concurrency guard). - * - * @param string $caseId Case UUID - * @param int $readVersion The @self.version captured at read time - * @param string $currentId The statusType UUID observed at read time - * - * @return array The freshly loaded case - * - * @throws RuntimeException When the case vanished or was concurrently changed - */ - private function assertNoConcurrentChange( - string $caseId, - int $readVersion, - string $currentId, - ): array { - $caseAtSave = $this->store->loadCase(caseId: $caseId); - if ($caseAtSave === null) { - throw new RuntimeException('case_not_found'); - } - - $versionAtSave = (int)(($caseAtSave['@self']['version'] ?? ($caseAtSave['version'] ?? 0))); - if ($versionAtSave !== $readVersion) { - throw new RefusedException( - rule: 'transition-conflict', - sentence: 'Another change reached this case first. Reload it and try again.', - status: RefusedException::STATUS_REFUSED, - ); - } - - $statusAtSave = (string)($caseAtSave['status'] ?? ''); - if ($statusAtSave !== $currentId) { - throw new RefusedException( - rule: 'transition-conflict', - sentence: 'Another change reached this case first. Reload it and try again.', - status: RefusedException::STATUS_REFUSED, - ); - } - - return $caseAtSave; - }//end assertNoConcurrentChange() - - /** - * Persist the dispatched-action results onto the statusRecord. - * - * @param array $record The statusRecord - * @param array> $dispatched Dispatch results - * @param string $statusRecordId The statusRecord UUID - * - * @return array The (possibly updated) statusRecord - */ - private function persistDispatchedActions( - array $record, - array $dispatched, - string $statusRecordId, - ): array { - if ($statusRecordId === '') { - return $record; - } - - $record['dispatchedActions'] = $dispatched; - try { - return $this->store->updateStatusRecord(record: $record); - } catch (\Throwable $e) { - $this->logger->error( - 'StatusTransitionService: dispatchedActions persist failed', - ['exception' => $e->getMessage(), 'statusRecord' => $statusRecordId], - ); - } - - return $record; - }//end persistDispatchedActions() - /** * Execute an admin-only free-form transition for caseTypes without an active workflow template. * @@ -771,7 +537,7 @@ public function executeFreeForm(string $caseId, string $toStatusId, ?string $com throw new RuntimeException('forbidden_admin_only'); } - $case = $this->store->loadCase(caseId: $caseId); + $case = $this->ledger->load(caseId: $caseId); if ($case === null) { throw new RuntimeException('case_not_found'); } @@ -790,44 +556,29 @@ public function executeFreeForm(string $caseId, string $toStatusId, ?string $com // would read complete and not be. $this->processOwnedStatus->requireHandSetAllowed(caseTypeId: $caseTypeId); - $this->store->assertStatusBelongsToCaseType(caseTypeId: $caseTypeId, statusTypeId: $toStatusId); - - $currentId = (string)($case['status'] ?? ''); - // The dwell bookkeeping belongs to the MOVE, not to the road into it. - // An admin free-form move is still a move, and a case whose dwell reset - // only on the guarded path would report months in a status it entered - // this morning. - $case = $this->declarations->applyStatusChange(case: $case, toStatus: $toStatusId); - $case['status'] = $toStatusId; - $case = $this->store->saveCase(case: $case); - $this->declarations->retime(caseId: $caseId, toStatus: $toStatusId); + $this->ledger->assertStatusOfCaseType(caseTypeId: $caseTypeId, statusTypeId: $toStatusId); - $record = $this->store->writeStatusRecord( - caseId: $caseId, + $move = [ + 'fromStatus' => (string)($case['status'] ?? ''), + 'toStatus' => $toStatusId, + 'label' => 'Free-form transition', + 'comment' => $comment, + 'actor' => $userId, + 'evaluatedGuards' => [], + 'noWorkflowTemplate' => true, + ]; + [$case] = $this->ledger->save( + case: $this->declarations->stamp(case: $case, toStatus: $toStatusId), toStatus: $toStatusId, - fromStatus: $currentId, - label: 'Free-form transition', - comment: $comment, - evaluatedGuards: [], - noWorkflowTemplate: true, - actor: $userId, ); + $this->declarations->retime(caseId: $caseId, toStatus: $toStatusId); - // A status brings its checklist however the case arrived. This path - // dispatched nothing at all before, because it has no transition to - // read actions off — but the work belongs to the phase, not to the road - // into it, so an admin's move brings the tasks too. Only the checklist - // actions run here: there is no transition whose actions could. - $dispatched = $this->sideEffectDispatcher->dispatch( - actions: $this->statusChecklist->actionsFor(statusTypeId: $toStatusId, case: $case, actor: $userId), + $record = $this->ledger->record(caseId: $caseId, move: $move); + $dispatched = $this->ledger->dispatch( + actions: $this->declarations->checklistActions(statusTypeId: $toStatusId, case: $case, actor: $userId), case: $case, - transitionContext: [ - 'fromStatus' => $currentId, - 'toStatus' => $toStatusId, - 'transitionLabel' => 'Free-form transition', - 'userId' => $userId, - 'statusRecordUuid' => (string)($record['id'] ?? ''), - ], + move: $move, + record: $record, ); $outcome = $this->actionOutcome(dispatched: $dispatched, caseId: $caseId); @@ -850,7 +601,7 @@ public function executeFreeForm(string $caseId, string $toStatusId, ?string $com * @spec openspec/specs/status-transition-engine/spec.md */ public function replay(string $caseId): array { - $list = $this->store->findStatusRecords(caseId: $caseId); + $list = $this->ledger->history(caseId: $caseId); if ($list === null) { return ['history' => [], 'replayable' => false]; } diff --git a/lib/Service/Transitions/CaseDeclarations.php b/lib/Service/Transitions/CaseDeclarations.php new file mode 100644 index 0000000000..8df5f7a252 --- /dev/null +++ b/lib/Service/Transitions/CaseDeclarations.php @@ -0,0 +1,187 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * @version GIT: + * + * @link https://conduction.nl + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Dossiq\Service\Transitions; + +use OCA\Dossiq\Exception\RefusedException; +use OCA\Dossiq\Service\Status\StatusDeclarations; + +/** + * One reader over what a status, a transition and a checklist declare. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ +class CaseDeclarations { + /** + * Constructor. + * + * @param StatusDeclarations $statuses What a status declares about itself. + * @param TransitionDeclarations $moves What a transition declares about itself. + * @param OfferedTransitions $offered Which moves a case offers and which it withholds. + * @param StatusChecklist $checklist The checklist a status brings with it. + * @param CaseStatusStore $store Reads the row of a status. + */ + public function __construct( + private readonly StatusDeclarations $statuses, + private readonly TransitionDeclarations $moves, + private readonly OfferedTransitions $offered, + private readonly StatusChecklist $checklist, + private readonly CaseStatusStore $store, + ) { + }//end __construct() + + /** + * The name, the colour and the description of a status, as its row holds them. + * + * @param string $statusTypeId The status. + * + * @return array{statusName: string, statusColour: string, statusDescription: string} + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function statusFacts(string $statusTypeId): array { + return [ + 'statusName' => $this->store->lookupStatusName(statusTypeId: $statusTypeId), + 'statusColour' => $this->store->lookupStatusColour(statusTypeId: $statusTypeId), + 'statusDescription' => $this->store->lookupStatusDescription(statusTypeId: $statusTypeId), + ]; + }//end statusFacts() + + /** + * What the status the case is in declares: who it waits on, how long it has lasted, how it was derived. + * + * @param array $case The case. + * + * @return array `waitingOn`, `dwell` and `derivation`. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function panel(array $case): array { + return $this->statuses->panelFor(case: $case); + }//end panel() + + /** + * Which of a workflow's transitions this case offers this user, and which it withholds and why. + * + * @param array $transitions The transitions of the workflow the case runs on. + * @param array $case The case. + * @param string $currentId The status the case is in. + * @param string $userId The user asking. + * + * @return array{transitions: array>, withheld: array>} + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function offered(array $transitions, array $case, string $currentId, string $userId): array { + return $this->offered->from(transitions: $transitions, case: $case, currentId: $currentId, userId: $userId); + }//end offered() + + /** + * Refuse a move its own declaration withholds: an open dependency first, then four eyes. + * + * @param array $transition The transition. + * @param array $case The case. + * @param string $caseId The case uuid. + * @param string $userId The user taking the move. + * + * @return void + * + * @throws RefusedException When something the move waits on is open, or four eyes refuses the user. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function assertOpen(array $transition, array $case, string $caseId, string $userId): void { + $withheld = $this->moves->withheldReasons(transition: $transition, case: $case); + if ($withheld !== []) { + throw new RefusedException( + rule: 'transition-dependency-open', + sentence: 'This move is waiting on ' . $withheld[0] . '.', + status: RefusedException::STATUS_REFUSED, + ); + } + + $refusal = $this->moves->fourEyesRefusal(transition: $transition, caseId: $caseId, userId: $userId); + if ($refusal !== null) { + throw new RefusedException( + rule: 'transition-four-eyes', + sentence: $this->moves->refusalSentence(refusal: $refusal), + status: RefusedException::STATUS_FORBIDDEN, + ); + } + }//end assertOpen() + + /** + * The case payload with the bookkeeping of the status it enters. Saved by the caller. + * + * @param array $case The case payload about to be saved. + * @param string $toStatus The status being entered. + * + * @return array + * + * @spec openspec/specs/doorlooptijd-dashboard/spec.md + */ + public function stamp(array $case, string $toStatus): array { + return $this->statuses->applyStatusChange(case: $case, toStatus: $toStatus); + }//end stamp() + + /** + * Stop the clock on the status the case left and start one on the status it entered. + * + * Called after the save, never before it. + * + * @param string $caseId The case uuid. + * @param string $toStatus The status entered. + * + * @return void + * + * @spec openspec/specs/doorlooptijd-dashboard/spec.md + */ + public function retime(string $caseId, string $toStatus): void { + $this->statuses->retime(caseId: $caseId, toStatus: $toStatus); + }//end retime() + + /** + * The actions the entered status brings with it. + * + * @param string $statusTypeId The status entered. + * @param array $case The saved case. + * @param string $actor The acting user. + * + * @return array> + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function checklistActions(string $statusTypeId, array $case, string $actor): array { + return $this->checklist->actionsFor(statusTypeId: $statusTypeId, case: $case, actor: $actor); + }//end checklistActions() +}//end class diff --git a/lib/Service/Transitions/CaseMoveLedger.php b/lib/Service/Transitions/CaseMoveLedger.php new file mode 100644 index 0000000000..5f7e1ff7ee --- /dev/null +++ b/lib/Service/Transitions/CaseMoveLedger.php @@ -0,0 +1,291 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * @version GIT: + * + * @link https://conduction.nl + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Dossiq\Service\Transitions; + +use OCA\Dossiq\Exception\RefusedException; +use Psr\Log\LoggerInterface; +use RuntimeException; + +/** + * The store, the closing result and the side effects of a move, behind one class. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ +class CaseMoveLedger { + /** + * Constructor. + * + * @param CaseStatusStore $store OpenRegister persistence for the engine. + * @param CaseResultWriter $resultWriter Closing-result reader and writer. + * @param SideEffectDispatcher $dispatcher Side-effect dispatcher. + * @param LoggerInterface $logger The logger. + */ + public function __construct( + private readonly CaseStatusStore $store, + private readonly CaseResultWriter $resultWriter, + private readonly SideEffectDispatcher $dispatcher, + private readonly LoggerInterface $logger, + ) { + }//end __construct() + + /** + * The case as stored, or null when it is not there. + * + * @param string $caseId The case uuid. + * + * @return array|null + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function load(string $caseId): ?array { + return $this->store->loadCase(caseId: $caseId); + }//end load() + + /** + * Re-load the case immediately before writing and refuse when another change landed in the meantime. + * + * @param string $caseId The case uuid. + * @param int $readVersion The `@self.version` captured at read time. + * @param string $currentId The status observed at read time. + * + * @return array The freshly loaded case. + * + * @throws RuntimeException When the case is gone. + * @throws RefusedException When its version or its status changed. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function reloadUnchanged(string $caseId, int $readVersion, string $currentId): array { + $caseAtSave = $this->store->loadCase(caseId: $caseId); + if ($caseAtSave === null) { + throw new RuntimeException('case_not_found'); + } + + $versionAtSave = (int)(($caseAtSave['@self']['version'] ?? ($caseAtSave['version'] ?? 0))); + $statusAtSave = (string)($caseAtSave['status'] ?? ''); + if ($versionAtSave !== $readVersion || $statusAtSave !== $currentId) { + throw new RefusedException( + rule: 'transition-conflict', + sentence: 'Another change reached this case first. Reload it and try again.', + status: RefusedException::STATUS_REFUSED, + ); + } + + return $caseAtSave; + }//end reloadUnchanged() + + /** + * Write the result a closing move records onto the case payload. Not saved here. + * + * Returns the case unchanged when the target status is not final. When it + * is, the end date is settled, and with a result the archival future too, + * so all of it lands in the one save of the move. + * + * @param array $case The case payload about to be saved. + * @param string $caseId The case uuid. + * @param string $toStatus The status being entered. + * @param string|null $resultTypeId The chosen result type, or null. + * + * @return array The payload, with `endDate`, `result` and the archival fields when closing. + * + * @throws RuntimeException When a closing move carries no result type. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function close(array $case, string $caseId, string $toStatus, ?string $resultTypeId): array { + if ($this->resultWriter->isFinalStatus(statusTypeId: $toStatus) === false) { + return $case; + } + + $endDate = substr((string)($case['endDate'] ?? ''), 0, 10); + if ($endDate === '') { + $endDate = date('Y-m-d'); + } + + $case['endDate'] = $endDate; + + $resultId = $this->resultWriter->resolveClosingResult( + caseId: $caseId, + caseTypeId: (string)($case['caseType'] ?? ''), + resultTypeId: $resultTypeId, + ); + if ($resultId === null) { + return $case; + } + + $case['result'] = $resultId; + + return array_merge( + $case, + $this->resultWriter->archivalFuture(case: $case, resultTypeId: (string)$resultTypeId, endDate: $endDate) + ); + }//end close() + + /** + * Save the case in the status it enters, once. + * + * With a read version the save carries it as `@self.version`, so the store + * refuses a write another move got in front of. + * + * @param array $case The settled case payload. + * @param string $toStatus The status being entered. + * @param int|null $readVersion The version captured at read time, or null to save without one. + * + * @return array{0: array, 1: int} The saved case and its new version. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function save(array $case, string $toStatus, ?int $readVersion = null): array { + $case['status'] = $toStatus; + if ($readVersion !== null) { + $self = ($case['@self'] ?? null); + if (is_array($self) === false) { + $self = []; + } + + $self['version'] = $readVersion; + $case['@self'] = $self; + } + + $saved = $this->store->saveCase(case: $case); + + return [$saved, (int)(($saved['@self']['version'] ?? ($saved['version'] ?? 0)))]; + }//end save() + + /** + * Refuse a status that is not one of the case type's. + * + * @param string $caseTypeId The case type. + * @param string $statusTypeId The status. + * + * @return void + * + * @throws RuntimeException When the status belongs to another case type. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function assertStatusOfCaseType(string $caseTypeId, string $statusTypeId): void { + $this->store->assertStatusBelongsToCaseType(caseTypeId: $caseTypeId, statusTypeId: $statusTypeId); + }//end assertStatusOfCaseType() + + /** + * Write the status record of a move. + * + * @param string $caseId The case uuid. + * @param array $move `fromStatus`, `toStatus`, `label`, `comment`, `actor`, `evaluatedGuards`, `noWorkflowTemplate`. + * + * @return array The status record. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function record(string $caseId, array $move): array { + return $this->store->writeStatusRecord( + caseId: $caseId, + toStatus: (string)$move['toStatus'], + fromStatus: (string)$move['fromStatus'], + label: (string)$move['label'], + comment: $move['comment'], + evaluatedGuards: $move['evaluatedGuards'], + noWorkflowTemplate: $move['noWorkflowTemplate'], + actor: (string)$move['actor'], + ); + }//end record() + + /** + * Run the actions of a move, in order, with the move and its record as their context. + * + * @param array> $actions The actions to run. + * @param array $case The saved case. + * @param array $move The move, as given to {@see self::record()}. + * @param array $record The status record of the move. + * + * @return array> One result row per action. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function dispatch(array $actions, array $case, array $move, array $record): array { + return $this->dispatcher->dispatch( + actions: $actions, + case: $case, + transitionContext: [ + 'fromStatus' => (string)$move['fromStatus'], + 'toStatus' => (string)$move['toStatus'], + 'transitionLabel' => (string)$move['label'], + 'userId' => (string)$move['actor'], + 'statusRecordUuid' => (string)($record['id'] ?? ''), + ] + ); + }//end dispatch() + + /** + * Write the results of the dispatched actions back onto the status record. + * + * A record without an id is left as it is. A write that fails is logged at + * error and the record is answered as it stood: the move has happened. + * + * @param array $record The status record. + * @param array> $dispatched The result rows. + * + * @return array The record, updated when it could be. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function writeBack(array $record, array $dispatched): array { + $statusRecordId = (string)($record['id'] ?? ''); + if ($statusRecordId === '') { + return $record; + } + + $record['dispatchedActions'] = $dispatched; + try { + return $this->store->updateStatusRecord(record: $record); + } catch (\Throwable $e) { + $this->logger->error( + 'StatusTransitionService: dispatchedActions persist failed', + ['exception' => $e->getMessage(), 'statusRecord' => $statusRecordId], + ); + } + + return $record; + }//end writeBack() + + /** + * The status records of a case, unsorted, or null when they cannot be read. + * + * @param string $caseId The case uuid. + * + * @return array>|null + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function history(string $caseId): ?array { + return $this->store->findStatusRecords(caseId: $caseId); + }//end history() +}//end class diff --git a/openspec/changes/method-decomposition/design.md b/openspec/changes/method-decomposition/design.md index 52a9777130..9cd862149d 100644 --- a/openspec/changes/method-decomposition/design.md +++ b/openspec/changes/method-decomposition/design.md @@ -78,7 +78,7 @@ A move is not a design here. Folding two collaborators into a third to lower the | The engine asks | Today it holds | After | |---|---|---| -| What does this case declare about its status and its moves? | `StatusDeclarations`, `TransitionDeclarations`, `OfferedTransitions`, `StatusChecklist` | `Transitions/CaseDeclarations` | +| What does this case declare about its status and its moves? | `StatusDeclarations`, `TransitionDeclarations`, `OfferedTransitions`, `StatusChecklist`, and the status row read of `CaseStatusStore` | `Transitions/CaseDeclarations` | | What does a move write, and what did earlier moves write? | `CaseStatusStore`, `CaseResultWriter`, `SideEffectDispatcher` | `Transitions/CaseMoveLedger` | | May this person take this move? | `TransitionAuthorizer`, `GuardRegistry`, `TransitionSpecReader`, `ProcessOwnedStatusRule` | unchanged, the engine keeps them | | Which workflow does the case run on, and who is asking? | `WorkflowTemplateLoader`, `IUserSession`, `LoggerInterface` | unchanged | @@ -87,11 +87,12 @@ The engine keeps the decision: which transition, in which order the checks run, ### `Transitions/CaseDeclarations` (the read side) -One class over the four that read what an administrator declared. No write to the store goes through it. +One class over the four that read what an administrator declared, plus the store for one read: the row of a status. No write to the store goes through it. | Method | Takes over | Answers | |---|---|---| | `panel(array $case): array` | `StatusDeclarations::panelFor` | `waitingOn`, `dwell`, `derivation` of the status the case is in | +| `statusFacts(string $statusTypeId): array` | `CaseStatusStore::lookupStatusName`, `lookupStatusColour`, `lookupStatusDescription` | one call, keys `statusName`, `statusColour`, `statusDescription` | | `offered(array $transitions, array $case, string $currentId, string $userId): array` | `OfferedTransitions::from` | `transitions` and `withheld` | | `assertOpen(array $transition, array $case, string $caseId, string $userId): void` | `TransitionDeclarations::withheldReasons`, `fourEyesRefusal`, `refusalSentence` and the two refusals the engine throws on them | throws `RefusedException` `transition-dependency-open` or `transition-four-eyes`, in that order | | `stamp(array $case, string $toStatus): array` | `StatusDeclarations::applyStatusChange` | the case payload with the dwell bookkeeping of the status it enters | @@ -107,18 +108,22 @@ One class over the three that touch the store or the outside world. It takes who | Method | Takes over | Notes | |---|---|---| | `load(string $caseId): ?array` | `CaseStatusStore::loadCase` | | -| `statusFacts(string $statusTypeId): array` | `lookupStatusName`, `lookupStatusColour`, `lookupStatusDescription` | one call, keys `statusName`, `statusColour`, `statusDescription` | | `reloadUnchanged(string $caseId, int $readVersion, string $currentId): array` | the engine's `assertNoConcurrentChange()` | throws `case_not_found` or `RefusedException` `transition-conflict` | -| `saveMove(array $case, string $caseId, string $toStatus, ?string $resultTypeId, int $readVersion): array` | the store half of `writeMove()` and all of `applyClosingResult()` | closing result, end date, archival future, status and `@self.version` in ONE save; returns the saved case and its version | -| `saveFreeForm(array $case, string $toStatus): array` | `assertStatusBelongsToCaseType` and the save of `executeFreeForm()` | no closing result, as today | -| `recordAndDispatch(array $case, array $move, array $actions): array` | `writeStatusRecord`, `SideEffectDispatcher::dispatch`, the engine's `persistDispatchedActions()` | `$move` is `fromStatus`, `toStatus`, `label`, `comment`, `actor`, `evaluatedGuards`, `noWorkflowTemplate`; returns the record and the dispatched rows | +| `close(array $case, string $caseId, string $toStatus, ?string $resultTypeId): array` | the engine's `applyClosingResult()` | end date, closing result and archival future on the PAYLOAD; saves nothing | +| `save(array $case, string $toStatus, ?int $readVersion = null): array` | the save of `writeMove()` and of `executeFreeForm()` | the status and, with a read version, `@self.version`, in ONE save; returns the saved case and its version | +| `assertStatusOfCaseType(string $caseTypeId, string $statusTypeId): void` | `CaseStatusStore::assertStatusBelongsToCaseType` | the free-form check | +| `record(string $caseId, array $move): array` | `CaseStatusStore::writeStatusRecord` | `$move` is `fromStatus`, `toStatus`, `label`, `comment`, `actor`, `evaluatedGuards`, `noWorkflowTemplate` | +| `dispatch(array $actions, array $case, array $move, array $record): array` | `SideEffectDispatcher::dispatch` | builds the transition context from the move and the record | +| `writeBack(array $record, array $dispatched): array` | the engine's `persistDispatchedActions()` | a failed write is logged at error, the record answered as it stood | | `history(string $caseId): ?array` | `findStatusRecords` | `replay()` keeps the sort | -`saveMove()` takes the payload AFTER `CaseDeclarations::stamp()`. The engine calls `stamp()`, then `saveMove()`, then `retime()`: the same order as today, and the only place the two facades meet. +The engine calls `close()`, then `CaseDeclarations::stamp()`, then `save()`, then `retime()`: the same order as before the split, and the only place the two facades meet. `close()` and `save()` are two methods because the stamp sits between them. A free-form move calls `record()` and `dispatch()` and never `writeBack()`, as before. + +As built, two things differ from the first draft of this design. The status row read (`statusFacts`) sits on the read side: it is a declaration, and with it the ledger stays at ten public methods, the PHPMD limit. And the ledger has no `saveMove()` or `recordAndDispatch()`: a single method each would have had to change the order of the stamp, or take a flag for the write-back. ### The engine after the split -Constructor, nine arguments: `WorkflowTemplateLoader`, `GuardRegistry`, `TransitionAuthorizer`, `TransitionSpecReader`, `IUserSession`, `LoggerInterface`, `ProcessOwnedStatusRule`, `CaseDeclarations`, `CaseMoveLedger`. With `RefusedException`, `GuardFailedException` and `RuntimeException` it couples to 12 classes. Both suppressions go and `phpmd.suppressWarnings` in `.debt-baseline.json` goes down by 2. +Constructor, nine arguments: `WorkflowTemplateLoader`, `GuardRegistry`, `TransitionAuthorizer`, `TransitionSpecReader`, `IUserSession`, `LoggerInterface`, `ProcessOwnedStatusRule`, `CaseDeclarations`, `CaseMoveLedger`. With `RefusedException`, `GuardFailedException` and `RuntimeException` it couples to 12 classes (measured: whole-tree PHPMD is clean without the suppressions). Both suppressions go and `phpmd.suppressWarnings` in `.debt-baseline.json` goes down by 2. The public methods and what they return do not change: `getAvailableTransitions()`, `execute()`, `executeFreeForm()`, `replay()`, `isAdmin()`, `ADMIN_GROUP_ID`. The container builds both facades by type, so no registration changes. Every class that injects the engine (32 files under `lib/`) is untouched. diff --git a/openspec/changes/method-decomposition/tasks.md b/openspec/changes/method-decomposition/tasks.md index 76d1d54f4a..0c266d83cd 100644 --- a/openspec/changes/method-decomposition/tasks.md +++ b/openspec/changes/method-decomposition/tasks.md @@ -493,13 +493,21 @@ characterisation test with a mapped ObjectService is a sufficient safety net; th active. Behaviour kept, not changed here. Own tests: `tests/Unit/Service/Zgw/ZtcConceptRulesTest` (6), `ZtcTypeReferencesTest` (4, every search asserted with its schema) and `ZtcPublishAndDeleteChecksTest` (6). - - [ ] 8h `StatusTransitionService` (2: CouplingBetweenObjects 18 against 13, and + - [x] 8h `StatusTransitionService` (2: CouplingBetweenObjects 18 against 13, and ExcessiveParameterList, 14 constructor arguments against 10). Not a move: the design is in - design.md, "StatusTransitionService: two facades". `Transitions/CaseDeclarations` over the - four classes that read what was declared, `Transitions/CaseMoveLedger` over the three - that write. The engine keeps the decision and the order of the checks. - - [x] Design written (design.md). - - [ ] Characterisation: `tests/Unit/Service/StatusTransitionServiceOrderTest.php`, the eight - points of the design, green on the single class, own commit. - - [ ] The two facades with their own tests, the engine delegating, both suppressions removed, - `phpmd.suppressWarnings` lowered by 2. + design.md, "StatusTransitionService: two facades". `Transitions/CaseDeclarations` reads + what a case declares about its status and its moves (over `StatusDeclarations`, + `TransitionDeclarations`, `OfferedTransitions`, `StatusChecklist` and the status row). + `Transitions/CaseMoveLedger` holds what a move writes (over `CaseStatusStore`, + `CaseResultWriter`, `SideEffectDispatcher`). The engine keeps the decision and the order + of the checks, takes nine arguments and couples to 12 classes. Both suppressions are gone + (183 to 181). The public methods and their answers are unchanged; the constructor changed. + Characterisation, green on the single class first (own commit): + `tests/Unit/Service/StatusTransitionServiceOrderTest.php`, 10 tests over the order of the + five refusals, the conflict check, the one save of a closing move with the version read, + the timer after the save, the record that cannot be written back, the free-form refusals + and the shape of the available transitions. Seven mutations of the engine turn it red. + Unchanged and green after the split apart from its one constructor helper, as are the + five existing engine tests. + Own tests: `tests/Unit/Service/Transitions/CaseDeclarationsTest` (4) and + `CaseMoveLedgerTest` (6). diff --git a/tests/Unit/Lifecycle/CaseActionProviderTest.php b/tests/Unit/Lifecycle/CaseActionProviderTest.php index 474ae972ce..61973fe02c 100644 --- a/tests/Unit/Lifecycle/CaseActionProviderTest.php +++ b/tests/Unit/Lifecycle/CaseActionProviderTest.php @@ -35,6 +35,8 @@ use OCP\App\IAppManager; use Psr\Container\ContainerInterface; use OCA\Dossiq\Service\StatusTransitionService; +use OCA\Dossiq\Service\Transitions\CaseDeclarations; +use OCA\Dossiq\Service\Transitions\CaseMoveLedger; use OCA\Dossiq\Service\Money\CasePaymentReader; use OCA\Dossiq\Service\Money\CasePaymentState; use OCA\Dossiq\Service\Money\UnpaidCaseGate; @@ -60,6 +62,8 @@ * Maps dossiq's transitions onto OpenRegister's published actions. * * @covers \OCA\Dossiq\Lifecycle\CaseActionProvider + * @uses \OCA\Dossiq\Service\Transitions\CaseDeclarations + * @uses \OCA\Dossiq\Service\Transitions\CaseMoveLedger * @uses \OCA\Dossiq\Lifecycle\CaseActionList * @uses \OCA\Dossiq\Service\StatusTransitionService * @uses \OCA\Dossiq\Service\Transitions\TransitionSpecReader @@ -339,22 +343,28 @@ public function testARoleHiddenMoveIsAbsent(): void { $engine = new StatusTransitionService( templateLoader: $templateLoader, guardRegistry: $guardRegistry, - sideEffectDispatcher: $this->createMock(SideEffectDispatcher::class), - store: $store, authorizer: $this->createMock(TransitionAuthorizer::class), specReader: new TransitionSpecReader(), userSession: $this->createMock(IUserSession::class), logger: $this->createMock(LoggerInterface::class), - resultWriter: $this->createMock(CaseResultWriter::class), - statusChecklist: $this->createMock(StatusChecklist::class), - declarations: $this->undeclaredStatuses(), - declaredMoves: $this->undeclaredTransitions(), - offered: $this->offeredTransitions( - guards: $guardRegistry, - reader: new TransitionSpecReader(), + processOwnedStatus: $this->createMock(originalClassName: ProcessOwnedStatusRule::class), + declarations: new CaseDeclarations( statuses: $this->undeclaredStatuses(), + moves: $this->undeclaredTransitions(), + offered: $this->offeredTransitions( + guards: $guardRegistry, + reader: new TransitionSpecReader(), + statuses: $this->undeclaredStatuses(), + ), + checklist: $this->createMock(StatusChecklist::class), + store: $store, + ), + ledger: new CaseMoveLedger( + store: $store, + resultWriter: $this->createMock(CaseResultWriter::class), + dispatcher: $this->createMock(SideEffectDispatcher::class), + logger: $this->createMock(LoggerInterface::class), ), - processOwnedStatus: $this->createMock(originalClassName: ProcessOwnedStatusRule::class), ); $provider = new CaseActionProvider( diff --git a/tests/Unit/Service/StatusTransitionServiceEnforcementTest.php b/tests/Unit/Service/StatusTransitionServiceEnforcementTest.php index d01e8a5e52..7674f3fa78 100644 --- a/tests/Unit/Service/StatusTransitionServiceEnforcementTest.php +++ b/tests/Unit/Service/StatusTransitionServiceEnforcementTest.php @@ -51,6 +51,8 @@ namespace OCA\Dossiq\Tests\Unit\Service; use OCA\Dossiq\Service\StatusTransitionService; +use OCA\Dossiq\Service\Transitions\CaseDeclarations; +use OCA\Dossiq\Service\Transitions\CaseMoveLedger; use OCA\Dossiq\Service\Transitions\CaseResultWriter; use OCA\Dossiq\Service\Transitions\CaseStatusStore; use OCA\Dossiq\Service\Transitions\GuardRegistry; @@ -73,6 +75,8 @@ * The role check and the side effects, asked of one execute() call. * * @covers \OCA\Dossiq\Service\StatusTransitionService + * @uses \OCA\Dossiq\Service\Transitions\CaseDeclarations + * @uses \OCA\Dossiq\Service\Transitions\CaseMoveLedger * @covers \OCA\Dossiq\Service\Transitions\TransitionAuthorizer * * `TransitionSpecReader` is a real collaborator here rather than a mock, so @@ -191,8 +195,6 @@ protected function setUp(): void { $this->service = new StatusTransitionService( templateLoader: $templateLoader, guardRegistry: $guardRegistry, - sideEffectDispatcher: $this->dispatcher, - store: $this->store, // The REAL gate, over a mocked membership lookup. A mocked // authorizer answering `true` is the shape that let the board's // bypass go unnoticed for as long as it did. @@ -203,16 +205,24 @@ protected function setUp(): void { specReader: new TransitionSpecReader(), userSession: $userSession, logger: $this->createMock(LoggerInterface::class), - resultWriter: $resultWriter, - statusChecklist: $checklist, - declarations: $this->undeclaredStatuses(), - declaredMoves: $this->undeclaredTransitions(), - offered: $this->offeredTransitions( - guards: $guardRegistry, - reader: new TransitionSpecReader(), + processOwnedStatus: $this->createMock(originalClassName: ProcessOwnedStatusRule::class), + declarations: new CaseDeclarations( statuses: $this->undeclaredStatuses(), + moves: $this->undeclaredTransitions(), + offered: $this->offeredTransitions( + guards: $guardRegistry, + reader: new TransitionSpecReader(), + statuses: $this->undeclaredStatuses(), + ), + checklist: $checklist, + store: $this->store, + ), + ledger: new CaseMoveLedger( + store: $this->store, + resultWriter: $resultWriter, + dispatcher: $this->dispatcher, + logger: $this->createMock(LoggerInterface::class), ), - processOwnedStatus: $this->createMock(originalClassName: ProcessOwnedStatusRule::class), ); }//end setUp() diff --git a/tests/Unit/Service/StatusTransitionServiceFailedActionsTest.php b/tests/Unit/Service/StatusTransitionServiceFailedActionsTest.php index 21f7f8b17d..7401129cb6 100644 --- a/tests/Unit/Service/StatusTransitionServiceFailedActionsTest.php +++ b/tests/Unit/Service/StatusTransitionServiceFailedActionsTest.php @@ -24,6 +24,8 @@ namespace OCA\Dossiq\Tests\Unit\Service; use OCA\Dossiq\Service\StatusTransitionService; +use OCA\Dossiq\Service\Transitions\CaseDeclarations; +use OCA\Dossiq\Service\Transitions\CaseMoveLedger; use OCA\Dossiq\Service\Transitions\CaseResultWriter; use OCA\Dossiq\Service\Transitions\CaseStatusStore; use OCA\Dossiq\Service\Transitions\GuardRegistry; @@ -44,6 +46,8 @@ * The answer names the actions that did not run. * * @covers \OCA\Dossiq\Service\StatusTransitionService + * @uses \OCA\Dossiq\Service\Transitions\CaseDeclarations + * @uses \OCA\Dossiq\Service\Transitions\CaseMoveLedger * @uses \OCA\Dossiq\Service\Transitions\OfferedTransitions * * @spec openspec/changes/transition-reports-failed-actions/specs/status-transition-engine/spec.md @@ -128,22 +132,28 @@ protected function setUp(): void { $this->service = new StatusTransitionService( templateLoader: $templateLoader, guardRegistry: $guardRegistry, - sideEffectDispatcher: $this->dispatcher, - store: $store, authorizer: $authorizer, specReader: $specReader, userSession: $this->createMock(originalClassName: IUserSession::class), logger: $this->logger, - resultWriter: $resultWriter, - statusChecklist: $this->createMock(originalClassName: StatusChecklist::class), - declarations: $this->undeclaredStatuses(), - declaredMoves: $this->undeclaredTransitions(), - offered: $this->offeredTransitions( - guards: $guardRegistry, - reader: $specReader, + processOwnedStatus: $this->createMock(originalClassName: ProcessOwnedStatusRule::class), + declarations: new CaseDeclarations( statuses: $this->undeclaredStatuses(), + moves: $this->undeclaredTransitions(), + offered: $this->offeredTransitions( + guards: $guardRegistry, + reader: $specReader, + statuses: $this->undeclaredStatuses(), + ), + checklist: $this->createMock(originalClassName: StatusChecklist::class), + store: $store, + ), + ledger: new CaseMoveLedger( + store: $store, + resultWriter: $resultWriter, + dispatcher: $this->dispatcher, + logger: $this->logger, ), - processOwnedStatus: $this->createMock(originalClassName: ProcessOwnedStatusRule::class), ); }//end setUp() diff --git a/tests/Unit/Service/StatusTransitionServiceOrderTest.php b/tests/Unit/Service/StatusTransitionServiceOrderTest.php index 3c6ec4ed6a..8c0ae92399 100644 --- a/tests/Unit/Service/StatusTransitionServiceOrderTest.php +++ b/tests/Unit/Service/StatusTransitionServiceOrderTest.php @@ -31,6 +31,8 @@ use OCA\Dossiq\Service\Lifecycle\ProcessOwnedStatusRule; use OCA\Dossiq\Service\Status\StatusDeclarations; use OCA\Dossiq\Service\StatusTransitionService; +use OCA\Dossiq\Service\Transitions\CaseDeclarations; +use OCA\Dossiq\Service\Transitions\CaseMoveLedger; use OCA\Dossiq\Service\Transitions\CaseResultWriter; use OCA\Dossiq\Service\Transitions\CaseStatusStore; use OCA\Dossiq\Service\Transitions\GuardFailedException; @@ -51,6 +53,8 @@ * The order of the engine's checks and writes. * * @covers \OCA\Dossiq\Service\StatusTransitionService + * @uses \OCA\Dossiq\Service\Transitions\CaseDeclarations + * @uses \OCA\Dossiq\Service\Transitions\CaseMoveLedger * @uses \OCA\Dossiq\Exception\RefusedException * @uses \OCA\Dossiq\Service\Transitions\GuardFailedException * @uses \OCA\Dossiq\Service\Transitions\TransitionSpecReader @@ -229,18 +233,24 @@ function (string $message): void { return new StatusTransitionService( templateLoader: $loader, guardRegistry: $guards, - sideEffectDispatcher: $dispatcher, - store: $store, authorizer: $authorizer, specReader: new TransitionSpecReader(), userSession: $this->createMock(IUserSession::class), logger: $logger, - resultWriter: $results, - statusChecklist: $checklist, - declarations: $statuses, - declaredMoves: $moves, - offered: $offered, processOwnedStatus: $handSet, + declarations: new CaseDeclarations( + statuses: $statuses, + moves: $moves, + offered: $offered, + checklist: $checklist, + store: $store, + ), + ledger: new CaseMoveLedger( + store: $store, + resultWriter: $results, + dispatcher: $dispatcher, + logger: $logger, + ), ); }//end engine() diff --git a/tests/Unit/Service/StatusTransitionServiceReplayRegressionTest.php b/tests/Unit/Service/StatusTransitionServiceReplayRegressionTest.php index 89dcb84c54..7a814973c8 100644 --- a/tests/Unit/Service/StatusTransitionServiceReplayRegressionTest.php +++ b/tests/Unit/Service/StatusTransitionServiceReplayRegressionTest.php @@ -27,6 +27,8 @@ use OCA\Dossiq\Service\CaseTypeStore; use OCA\Dossiq\Service\SettingsService; use OCA\Dossiq\Service\StatusTransitionService; +use OCA\Dossiq\Service\Transitions\CaseDeclarations; +use OCA\Dossiq\Service\Transitions\CaseMoveLedger; use OCA\Dossiq\Service\Archival\ArchivalBaseDateResolver; use OCA\Dossiq\Service\Archival\ArchivalNominationDeriver; use OCA\Dossiq\Service\Transitions\CaseResultWriter; @@ -61,6 +63,8 @@ public function searchObjects(array $query): array; * Regression tests for StatusTransitionService::replay(). * * @covers \OCA\Dossiq\Service\StatusTransitionService + * @uses \OCA\Dossiq\Service\Transitions\CaseDeclarations + * @uses \OCA\Dossiq\Service\Transitions\CaseMoveLedger * @uses \OCA\Dossiq\Service\Archival\ArchivalBaseDateResolver * @uses \OCA\Dossiq\Service\Archival\ArchivalNominationDeriver * @uses \OCA\Dossiq\Service\CaseTypeResolver @@ -106,24 +110,30 @@ protected function setUp(): void { $this->logger = $this->createMock(LoggerInterface::class); $this->service = new StatusTransitionService( - $this->createMock(WorkflowTemplateLoader::class), - $this->createMock(GuardRegistry::class), - $this->createMock(SideEffectDispatcher::class), - new CaseStatusStore($this->settingsService, new StatusTypeLookup($this->settingsService, new CaseTypeResolver(new CaseTypeStore($this->settingsService))), $this->logger), - new TransitionAuthorizer($this->createMock(IGroupManager::class), $this->logger), - new TransitionSpecReader(), - $this->createMock(IUserSession::class), - $this->logger, - new CaseResultWriter($this->settingsService, new CaseTypeResolver(new CaseTypeStore($this->settingsService)), new ArchivalNominationDeriver($this->settingsService, new ArchivalBaseDateResolver($this->settingsService), $this->logger)), - $this->createMock(StatusChecklist::class), - $this->undeclaredStatuses(), - $this->undeclaredTransitions(), - $this->offeredTransitions( - guards: $this->createMock(originalClassName: GuardRegistry::class), - reader: new TransitionSpecReader(), + templateLoader: $this->createMock(WorkflowTemplateLoader::class), + guardRegistry: $this->createMock(GuardRegistry::class), + authorizer: new TransitionAuthorizer($this->createMock(IGroupManager::class), $this->logger), + specReader: new TransitionSpecReader(), + userSession: $this->createMock(IUserSession::class), + logger: $this->logger, + processOwnedStatus: $this->createMock(originalClassName: ProcessOwnedStatusRule::class), + declarations: new CaseDeclarations( statuses: $this->undeclaredStatuses(), + moves: $this->undeclaredTransitions(), + offered: $this->offeredTransitions( + guards: $this->createMock(originalClassName: GuardRegistry::class), + reader: new TransitionSpecReader(), + statuses: $this->undeclaredStatuses(), + ), + checklist: $this->createMock(StatusChecklist::class), + store: new CaseStatusStore($this->settingsService, new StatusTypeLookup($this->settingsService, new CaseTypeResolver(new CaseTypeStore($this->settingsService))), $this->logger), + ), + ledger: new CaseMoveLedger( + store: new CaseStatusStore($this->settingsService, new StatusTypeLookup($this->settingsService, new CaseTypeResolver(new CaseTypeStore($this->settingsService))), $this->logger), + resultWriter: new CaseResultWriter($this->settingsService, new CaseTypeResolver(new CaseTypeStore($this->settingsService)), new ArchivalNominationDeriver($this->settingsService, new ArchivalBaseDateResolver($this->settingsService), $this->logger)), + dispatcher: $this->createMock(SideEffectDispatcher::class), + logger: $this->logger, ), - processOwnedStatus: $this->createMock(originalClassName: ProcessOwnedStatusRule::class), ); }//end setUp() diff --git a/tests/Unit/Service/StatusTransitionServiceResultTest.php b/tests/Unit/Service/StatusTransitionServiceResultTest.php index 54439511a6..b6c9df9aec 100644 --- a/tests/Unit/Service/StatusTransitionServiceResultTest.php +++ b/tests/Unit/Service/StatusTransitionServiceResultTest.php @@ -26,6 +26,8 @@ namespace OCA\Dossiq\Tests\Unit\Service; use OCA\Dossiq\Service\StatusTransitionService; +use OCA\Dossiq\Service\Transitions\CaseDeclarations; +use OCA\Dossiq\Service\Transitions\CaseMoveLedger; use OCA\Dossiq\Service\Transitions\CaseResultWriter; use OCA\Dossiq\Service\Transitions\CaseStatusStore; use OCA\Dossiq\Service\Transitions\GuardRegistry; @@ -47,6 +49,8 @@ * A final transition writes a result, or it does not happen. * * @covers \OCA\Dossiq\Service\StatusTransitionService + * @uses \OCA\Dossiq\Service\Transitions\CaseDeclarations + * @uses \OCA\Dossiq\Service\Transitions\CaseMoveLedger * @uses \OCA\Dossiq\Service\Transitions\OfferedTransitions */ class StatusTransitionServiceResultTest extends TestCase { @@ -131,22 +135,28 @@ protected function setUp(): void { $this->service = new StatusTransitionService( templateLoader: $this->templateLoader, guardRegistry: $guardRegistry, - sideEffectDispatcher: $dispatcher, - store: $this->store, authorizer: $authorizer, specReader: $specReader, userSession: $this->createMock(IUserSession::class), logger: $this->createMock(LoggerInterface::class), - resultWriter: $this->resultWriter, - statusChecklist: $this->createMock(StatusChecklist::class), - declarations: $this->undeclaredStatuses(), - declaredMoves: $this->undeclaredTransitions(), - offered: $this->offeredTransitions( - guards: $guardRegistry, - reader: $specReader, + processOwnedStatus: $this->createMock(originalClassName: ProcessOwnedStatusRule::class), + declarations: new CaseDeclarations( statuses: $this->undeclaredStatuses(), + moves: $this->undeclaredTransitions(), + offered: $this->offeredTransitions( + guards: $guardRegistry, + reader: $specReader, + statuses: $this->undeclaredStatuses(), + ), + checklist: $this->createMock(StatusChecklist::class), + store: $this->store, + ), + ledger: new CaseMoveLedger( + store: $this->store, + resultWriter: $this->resultWriter, + dispatcher: $dispatcher, + logger: $this->createMock(LoggerInterface::class), ), - processOwnedStatus: $this->createMock(originalClassName: ProcessOwnedStatusRule::class), ); }//end setUp() diff --git a/tests/Unit/Service/StatusTransitionServiceRouteSeamTest.php b/tests/Unit/Service/StatusTransitionServiceRouteSeamTest.php index ab32ba778b..46938aed3e 100644 --- a/tests/Unit/Service/StatusTransitionServiceRouteSeamTest.php +++ b/tests/Unit/Service/StatusTransitionServiceRouteSeamTest.php @@ -30,6 +30,8 @@ use OCA\Dossiq\Service\CaseTypeStore; use OCA\Dossiq\Service\SettingsService; use OCA\Dossiq\Service\StatusTransitionService; +use OCA\Dossiq\Service\Transitions\CaseDeclarations; +use OCA\Dossiq\Service\Transitions\CaseMoveLedger; use OCA\Dossiq\Service\Archival\ArchivalBaseDateResolver; use OCA\Dossiq\Service\Archival\ArchivalNominationDeriver; use OCA\Dossiq\Service\Transitions\CaseResultWriter; @@ -68,6 +70,8 @@ public function saveObject(array $object, mixed $register = null, mixed $schema * The engine resolves the workflow through the case. * * @covers \OCA\Dossiq\Service\StatusTransitionService + * @uses \OCA\Dossiq\Service\Transitions\CaseDeclarations + * @uses \OCA\Dossiq\Service\Transitions\CaseMoveLedger * @uses \OCA\Dossiq\Service\Archival\ArchivalBaseDateResolver * @uses \OCA\Dossiq\Service\Archival\ArchivalNominationDeriver * @uses \OCA\Dossiq\Service\CaseTypeResolver @@ -182,24 +186,30 @@ protected function setUp(): void { $this->groupManager = $this->createMock(IGroupManager::class); $this->service = new StatusTransitionService( - $this->templateLoader, - $this->guardRegistry, - $this->dispatcher, - new CaseStatusStore($this->settingsService, new StatusTypeLookup($this->settingsService, new CaseTypeResolver(new CaseTypeStore($this->settingsService))), $logger), - new TransitionAuthorizer($this->groupManager, $logger), - new TransitionSpecReader(), - $this->createMock(IUserSession::class), - $logger, - new CaseResultWriter($this->settingsService, new CaseTypeResolver(new CaseTypeStore($this->settingsService)), new ArchivalNominationDeriver($this->settingsService, new ArchivalBaseDateResolver($this->settingsService), $logger)), - $this->statusChecklist, - $this->undeclaredStatuses(), - $this->undeclaredTransitions(), - $this->offeredTransitions( - guards: $this->guardRegistry, - reader: new TransitionSpecReader(), + templateLoader: $this->templateLoader, + guardRegistry: $this->guardRegistry, + authorizer: new TransitionAuthorizer($this->groupManager, $logger), + specReader: new TransitionSpecReader(), + userSession: $this->createMock(IUserSession::class), + logger: $logger, + processOwnedStatus: $this->createMock(originalClassName: ProcessOwnedStatusRule::class), + declarations: new CaseDeclarations( statuses: $this->undeclaredStatuses(), + moves: $this->undeclaredTransitions(), + offered: $this->offeredTransitions( + guards: $this->guardRegistry, + reader: new TransitionSpecReader(), + statuses: $this->undeclaredStatuses(), + ), + checklist: $this->statusChecklist, + store: new CaseStatusStore($this->settingsService, new StatusTypeLookup($this->settingsService, new CaseTypeResolver(new CaseTypeStore($this->settingsService))), $logger), + ), + ledger: new CaseMoveLedger( + store: new CaseStatusStore($this->settingsService, new StatusTypeLookup($this->settingsService, new CaseTypeResolver(new CaseTypeStore($this->settingsService))), $logger), + resultWriter: new CaseResultWriter($this->settingsService, new CaseTypeResolver(new CaseTypeStore($this->settingsService)), new ArchivalNominationDeriver($this->settingsService, new ArchivalBaseDateResolver($this->settingsService), $logger)), + dispatcher: $this->dispatcher, + logger: $logger, ), - processOwnedStatus: $this->createMock(originalClassName: ProcessOwnedStatusRule::class), ); }//end setUp() diff --git a/tests/Unit/Service/Transitions/CaseDeclarationsTest.php b/tests/Unit/Service/Transitions/CaseDeclarationsTest.php new file mode 100644 index 0000000000..8af2fdf69c --- /dev/null +++ b/tests/Unit/Service/Transitions/CaseDeclarationsTest.php @@ -0,0 +1,174 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Dossiq\Tests\Unit\Service\Transitions; + +use OCA\Dossiq\Exception\RefusedException; +use OCA\Dossiq\Service\Status\StatusDeclarations; +use OCA\Dossiq\Service\Transitions\CaseDeclarations; +use OCA\Dossiq\Service\Transitions\CaseStatusStore; +use OCA\Dossiq\Service\Transitions\OfferedTransitions; +use OCA\Dossiq\Service\Transitions\StatusChecklist; +use OCA\Dossiq\Service\Transitions\TransitionDeclarations; +use PHPUnit\Framework\TestCase; + +/** + * What a case declares about its status and its moves, read through one class. + * + * @covers \OCA\Dossiq\Service\Transitions\CaseDeclarations + * @uses \OCA\Dossiq\Exception\RefusedException + * + * @spec openspec/changes/method-decomposition/tasks.md + */ +class CaseDeclarationsTest extends TestCase { + private const CASE = ['id' => 'case-1', 'status' => 'st-open']; + + /** + * What the four readers were asked, in order. + * + * @var array + */ + private array $asked = []; + + /** + * Build the reader over four doubles that record what they are asked. + * + * @param array $withheld What the move waits on. + * @param array|null $fourEyes The four-eyes refusal, or null. + * + * @return CaseDeclarations + */ + private function declarations(array $withheld = [], ?array $fourEyes = null): CaseDeclarations { + $record = function (string $name, mixed $answer): \Closure { + return function (...$arguments) use ($name, $answer): mixed { + $this->asked[] = [$name, $arguments]; + return $answer; + }; + }; + + $statuses = $this->createMock(StatusDeclarations::class); + $statuses->method('panelFor')->willReturnCallback($record('panelFor', ['waitingOn' => 'us'])); + $statuses->method('applyStatusChange')->willReturnCallback($record('applyStatusChange', ['stamped' => true])); + $statuses->method('retime')->willReturnCallback($record('retime', 'timer-1')); + + $moves = $this->createMock(TransitionDeclarations::class); + $moves->method('withheldReasons')->willReturnCallback($record('withheldReasons', $withheld)); + $moves->method('fourEyesRefusal')->willReturnCallback($record('fourEyesRefusal', $fourEyes)); + $moves->method('refusalSentence')->willReturnCallback($record('refusalSentence', 'Somebody else has to take this move.')); + + $offered = $this->createMock(OfferedTransitions::class); + $offered->method('from')->willReturnCallback($record('from', ['transitions' => [['id' => 't1']], 'withheld' => []])); + + $checklist = $this->createMock(StatusChecklist::class); + $checklist->method('actionsFor')->willReturnCallback($record('actionsFor', [['type' => 'createTask']])); + + $store = $this->createMock(CaseStatusStore::class); + $store->method('lookupStatusName')->willReturnCallback(static fn (string $statusTypeId): string => 'name of '.$statusTypeId); + $store->method('lookupStatusColour')->willReturn('#123456'); + $store->method('lookupStatusDescription')->willReturn('What it means.'); + $store->expects($this->never())->method('saveCase'); + + return new CaseDeclarations(statuses: $statuses, moves: $moves, offered: $offered, checklist: $checklist, store: $store); + }//end declarations() + + /** + * Every question goes to the reader that owns it, with what it was given, and its answer comes back. + * + * @return void + */ + public function testEveryQuestionGoesToItsReader(): void { + $declarations = $this->declarations(); + + $this->assertSame(['waitingOn' => 'us'], $declarations->panel(case: self::CASE)); + $this->assertSame( + ['statusName' => 'name of st-open', 'statusColour' => '#123456', 'statusDescription' => 'What it means.'], + $declarations->statusFacts(statusTypeId: 'st-open') + ); + $this->assertSame( + ['transitions' => [['id' => 't1']], 'withheld' => []], + $declarations->offered(transitions: [['id' => 't1']], case: self::CASE, currentId: 'st-open', userId: 'jan') + ); + $this->assertSame(['stamped' => true], $declarations->stamp(case: self::CASE, toStatus: 'st-closed')); + $declarations->retime(caseId: 'case-1', toStatus: 'st-closed'); + $this->assertSame([['type' => 'createTask']], $declarations->checklistActions(statusTypeId: 'st-closed', case: self::CASE, actor: 'jan')); + + $this->assertSame( + [ + ['panelFor', [self::CASE, null]], + ['from', [[['id' => 't1']], self::CASE, 'st-open', 'jan']], + ['applyStatusChange', [self::CASE, 'st-closed', null]], + ['retime', ['case-1', 'st-closed']], + ['actionsFor', ['st-closed', self::CASE, 'jan']], + ], + $this->asked + ); + }//end testEveryQuestionGoesToItsReader() + + /** + * A move nothing withholds is open: both declarations are asked, nothing is thrown. + * + * @return void + */ + public function testAnUnwithheldMoveIsOpen(): void { + $this->declarations()->assertOpen(transition: ['id' => 't1'], case: self::CASE, caseId: 'case-1', userId: 'jan'); + + $this->assertSame(['withheldReasons', 'fourEyesRefusal'], array_column($this->asked, 0)); + }//end testAnUnwithheldMoveIsOpen() + + /** + * An open dependency refuses first and names the first thing waited on; four eyes is not asked. + * + * @return void + */ + public function testAnOpenDependencyRefusesBeforeFourEyes(): void { + try { + $this->declarations(withheld: ['the advice', 'the fee'], fourEyes: ['reason' => 'same-person']) + ->assertOpen(transition: ['id' => 't1'], case: self::CASE, caseId: 'case-1', userId: 'jan'); + $this->fail('an open dependency must refuse'); + } catch (RefusedException $refused) { + $this->assertSame('transition-dependency-open', $refused->getRule()); + $this->assertSame('This move is waiting on the advice.', $refused->getSentence()); + $this->assertSame(RefusedException::STATUS_REFUSED, $refused->getStatus()); + } + + $this->assertSame(['withheldReasons'], array_column($this->asked, 0)); + }//end testAnOpenDependencyRefusesBeforeFourEyes() + + /** + * Four eyes refuses with the sentence its declaration gives, as forbidden. + * + * @return void + */ + public function testFourEyesRefusesWithItsOwnSentence(): void { + try { + $this->declarations(fourEyes: ['reason' => 'same-person']) + ->assertOpen(transition: ['id' => 't1'], case: self::CASE, caseId: 'case-1', userId: 'jan'); + $this->fail('four eyes must refuse'); + } catch (RefusedException $refused) { + $this->assertSame('transition-four-eyes', $refused->getRule()); + $this->assertSame('Somebody else has to take this move.', $refused->getSentence()); + $this->assertSame(RefusedException::STATUS_FORBIDDEN, $refused->getStatus()); + } + + $this->assertSame([['id' => 't1'], 'case-1', 'jan'], $this->asked[1][1]); + }//end testFourEyesRefusesWithItsOwnSentence() +}//end class diff --git a/tests/Unit/Service/Transitions/CaseMoveLedgerTest.php b/tests/Unit/Service/Transitions/CaseMoveLedgerTest.php new file mode 100644 index 0000000000..f9a3e93055 --- /dev/null +++ b/tests/Unit/Service/Transitions/CaseMoveLedgerTest.php @@ -0,0 +1,293 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Dossiq\Tests\Unit\Service\Transitions; + +use OCA\Dossiq\Exception\RefusedException; +use OCA\Dossiq\Service\Transitions\CaseMoveLedger; +use OCA\Dossiq\Service\Transitions\CaseResultWriter; +use OCA\Dossiq\Service\Transitions\CaseStatusStore; +use OCA\Dossiq\Service\Transitions\SideEffectDispatcher; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; +use RuntimeException; + +/** + * What a move writes: the conflict check, the closing result, the one save, the record and its side effects. + * + * @covers \OCA\Dossiq\Service\Transitions\CaseMoveLedger + * @uses \OCA\Dossiq\Exception\RefusedException + * + * @spec openspec/changes/method-decomposition/tasks.md + */ +class CaseMoveLedgerTest extends TestCase { + private const MOVE = [ + 'fromStatus' => 'st-open', + 'toStatus' => 'st-closed', + 'label' => 'Besluiten', + 'comment' => 'Klaar.', + 'actor' => 'jan', + 'evaluatedGuards' => [['type' => 'role', 'passed' => true]], + 'noWorkflowTemplate' => false, + ]; + + /** + * What the store, the result writer and the dispatcher were given. + * + * @var array + */ + private array $seen = []; + + /** + * The errors logged. + * + * @var array + */ + private array $errors = []; + + /** + * Build the ledger over doubles. + * + * @param array|null $stored The case the store answers, or null for none. + * @param bool $final Whether the status entered is final. + * @param string|null $result The closing result the writer resolves. + * @param \Throwable|null $updateFails What updating a status record throws. + * + * @return CaseMoveLedger + */ + private function ledger(?array $stored = null, bool $final = false, ?string $result = 'result-1', ?\Throwable $updateFails = null): CaseMoveLedger { + $store = $this->createMock(CaseStatusStore::class); + $store->method('loadCase')->willReturn($stored); + $store->method('saveCase')->willReturnCallback( + function (array $case): array { + $this->seen[] = ['saveCase', $case]; + return (['@self' => ['version' => 9]] + $case); + } + ); + $store->method('writeStatusRecord')->willReturnCallback( + function (...$arguments): array { + $this->seen[] = ['writeStatusRecord', $arguments]; + return ['id' => 'rec-1']; + } + ); + $store->method('updateStatusRecord')->willReturnCallback( + static function (array $record) use ($updateFails): array { + if ($updateFails !== null) { + throw $updateFails; + } + + return ($record + ['updated' => true]); + } + ); + $store->method('findStatusRecords')->willReturn([['id' => 'rec-0']]); + $store->method('assertStatusBelongsToCaseType')->willReturnCallback( + function (string $caseTypeId, string $statusTypeId): void { + $this->seen[] = ['assertStatusBelongsToCaseType', $caseTypeId, $statusTypeId]; + } + ); + + $results = $this->createMock(CaseResultWriter::class); + $results->method('isFinalStatus')->willReturn($final); + $results->method('resolveClosingResult')->willReturnCallback( + function (string $caseId, string $caseTypeId, ?string $resultTypeId) use ($result): ?string { + $this->seen[] = ['resolveClosingResult', $caseId, $caseTypeId, $resultTypeId]; + return $result; + } + ); + $results->method('archivalFuture')->willReturnCallback( + static fn (array $case, string $resultTypeId, string $endDate): array => ['archiveNomination' => 'vernietigen', 'archivedFor' => $resultTypeId.' '.$endDate] + ); + + $dispatcher = $this->createMock(SideEffectDispatcher::class); + $dispatcher->method('dispatch')->willReturnCallback( + function (array $actions, array $case, array $transitionContext): array { + $this->seen[] = ['dispatch', $actions, $case, $transitionContext]; + return [['type' => 'notify', 'ok' => true]]; + } + ); + + $logger = $this->createMock(LoggerInterface::class); + $logger->method('error')->willReturnCallback( + function (string $message, array $context = []): void { + $this->errors[] = [$message, $context]; + } + ); + + return new CaseMoveLedger(store: $store, resultWriter: $results, dispatcher: $dispatcher, logger: $logger); + }//end ledger() + + /** + * The stored case and the history come from the store. + * + * @return void + */ + public function testItReadsTheStore(): void { + $ledger = $this->ledger(stored: ['id' => 'case-1']); + + $this->assertSame(['id' => 'case-1'], $ledger->load(caseId: 'case-1')); + $this->assertNull($this->ledger()->load(caseId: 'case-1')); + $this->assertSame([['id' => 'rec-0']], $ledger->history(caseId: 'case-1')); + + $ledger->assertStatusOfCaseType(caseTypeId: 'ct-1', statusTypeId: 'st-open'); + $this->assertSame([['assertStatusBelongsToCaseType', 'ct-1', 'st-open']], $this->seen); + }//end testItReadsTheStore() + + /** + * A case that is as it was read is handed back; another version, another status or no case is not. + * + * @return void + */ + public function testAChangedCaseIsAConflict(): void { + $case = ['id' => 'case-1', 'status' => 'st-open', '@self' => ['version' => 3]]; + + $this->assertSame($case, $this->ledger(stored: $case)->reloadUnchanged(caseId: 'case-1', readVersion: 3, currentId: 'st-open')); + $this->assertSame( + ['id' => 'case-1', 'status' => 'st-open', 'version' => 3], + $this->ledger(stored: ['id' => 'case-1', 'status' => 'st-open', 'version' => 3])->reloadUnchanged(caseId: 'case-1', readVersion: 3, currentId: 'st-open') + ); + + $refused = []; + foreach ([[4, 'st-open'], [3, 'st-other']] as [$version, $status]) { + try { + $this->ledger(stored: $case)->reloadUnchanged(caseId: 'case-1', readVersion: $version, currentId: $status); + } catch (RefusedException $conflict) { + $refused[] = [$conflict->getRule(), $conflict->getStatus(), $conflict->getSentence()]; + } + } + + $sentence = 'Another change reached this case first. Reload it and try again.'; + $this->assertSame([['transition-conflict', 409, $sentence], ['transition-conflict', 409, $sentence]], $refused); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('case_not_found'); + $this->ledger()->reloadUnchanged(caseId: 'case-1', readVersion: 3, currentId: 'st-open'); + }//end testAChangedCaseIsAConflict() + + /** + * Closing settles the end date, the result and the archival future on the payload and saves nothing. + * + * Controls: a status that is not final leaves the payload alone, and a close without a result + * sets the end date only. + * + * @return void + */ + public function testClosingSettlesThePayload(): void { + $case = ['id' => 'case-1', 'caseType' => 'ct-1', 'endDate' => '2026-10-01T09:00:00+02:00']; + + $this->assertSame($case, $this->ledger()->close(case: $case, caseId: 'case-1', toStatus: 'st-open', resultTypeId: 'rt-1')); + $this->assertSame( + ['id' => 'case-1', 'caseType' => 'ct-1', 'endDate' => '2026-10-01'], + $this->ledger(final: true, result: null)->close(case: $case, caseId: 'case-1', toStatus: 'st-closed', resultTypeId: null) + ); + $this->assertSame( + [ + 'id' => 'case-1', + 'caseType' => 'ct-1', + 'endDate' => '2026-10-01', + 'result' => 'result-1', + 'archiveNomination' => 'vernietigen', + 'archivedFor' => 'rt-1 2026-10-01', + ], + $this->ledger(final: true)->close(case: $case, caseId: 'case-1', toStatus: 'st-closed', resultTypeId: 'rt-1') + ); + $this->assertSame( + date('Y-m-d'), + $this->ledger(final: true)->close(case: ['id' => 'case-1'], caseId: 'case-1', toStatus: 'st-closed', resultTypeId: 'rt-1')['endDate'] + ); + $this->assertSame([], array_filter($this->seen, static fn (array $call): bool => $call[0] === 'saveCase')); + $this->assertSame(['resolveClosingResult', 'case-1', 'ct-1', 'rt-1'], $this->seen[1]); + }//end testClosingSettlesThePayload() + + /** + * One save: the status entered, and with a read version that version as `@self.version`. + * + * @return void + */ + public function testTheSaveCarriesTheStatusAndTheVersionRead(): void { + $ledger = $this->ledger(); + + $withVersion = $ledger->save(case: ['id' => 'case-1', 'status' => 'st-open', '@self' => ['version' => 3, 'owner' => 'jan']], toStatus: 'st-closed', readVersion: 3); + $noSelf = $ledger->save(case: ['id' => 'case-1', '@self' => 'broken'], toStatus: 'st-closed', readVersion: 7); + $freeForm = $ledger->save(case: ['id' => 'case-1', 'status' => 'st-open'], toStatus: 'st-other'); + + $this->assertSame(['id' => 'case-1', 'status' => 'st-closed', '@self' => ['version' => 3, 'owner' => 'jan']], $this->seen[0][1]); + $this->assertSame(['id' => 'case-1', '@self' => ['version' => 7], 'status' => 'st-closed'], $this->seen[1][1]); + $this->assertSame(['id' => 'case-1', 'status' => 'st-other'], $this->seen[2][1], 'without a read version none is sent'); + $this->assertSame(9, $withVersion[1]); + $this->assertSame('st-closed', $noSelf[0]['status']); + $this->assertSame(9, $freeForm[1]); + }//end testTheSaveCarriesTheStatusAndTheVersionRead() + + /** + * The record is written from the move, and the actions run with the move and the record as context. + * + * @return void + */ + public function testTheRecordAndTheSideEffectsFollowTheMove(): void { + $ledger = $this->ledger(); + + $record = $ledger->record(caseId: 'case-1', move: self::MOVE); + $dispatched = $ledger->dispatch(actions: [['type' => 'notify']], case: ['id' => 'case-1'], move: self::MOVE, record: $record); + + $this->assertSame(['id' => 'rec-1'], $record); + $this->assertSame( + ['case-1', 'st-closed', 'st-open', 'Besluiten', 'Klaar.', [['type' => 'role', 'passed' => true]], false, 'jan'], + $this->seen[0][1] + ); + $this->assertSame([['type' => 'notify', 'ok' => true]], $dispatched); + $this->assertSame( + [ + 'dispatch', + [['type' => 'notify']], + ['id' => 'case-1'], + ['fromStatus' => 'st-open', 'toStatus' => 'st-closed', 'transitionLabel' => 'Besluiten', 'userId' => 'jan', 'statusRecordUuid' => 'rec-1'], + ], + $this->seen[1] + ); + }//end testTheRecordAndTheSideEffectsFollowTheMove() + + /** + * The results go back onto the record. A record without an id is left alone, and a write that + * fails is logged at error and answers the record as it stood. + * + * @return void + */ + public function testTheResultsAreWrittenBackOrLogged(): void { + $rows = [['type' => 'notify', 'ok' => true]]; + + $this->assertSame( + ['id' => 'rec-1', 'dispatchedActions' => $rows, 'updated' => true], + $this->ledger()->writeBack(record: ['id' => 'rec-1'], dispatched: $rows) + ); + $this->assertSame(['label' => 'x'], $this->ledger()->writeBack(record: ['label' => 'x'], dispatched: $rows)); + $this->assertSame([], $this->errors); + + $this->assertSame( + ['id' => 'rec-1', 'dispatchedActions' => $rows], + $this->ledger(updateFails: new RuntimeException('store down'))->writeBack(record: ['id' => 'rec-1'], dispatched: $rows) + ); + $this->assertSame( + [['StatusTransitionService: dispatchedActions persist failed', ['exception' => 'store down', 'statusRecord' => 'rec-1']]], + $this->errors + ); + }//end testTheResultsAreWrittenBackOrLogged() +}//end class From 6fdf35680aed1ae383748c04ba809727655dae02 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 12 Oct 2026 21:03:59 +0200 Subject: [PATCH 5/8] docs(spec): status citations point at the specs that hold the requirements The change what-a-status-declares was archived on 7 October. 206 @spec lines in 42 files still cited its path under openspec/changes/, which no longer exists. They now cite openspec/specs/status-transition-engine/spec.md (REQ-SDC-01 to 05) and openspec/specs/doorlooptijd-dashboard/spec.md (REQ-SDW-01), where the requirements live. Two lines that became a repeat of the line above them are removed. --- .../DerivedStatusListenerRegistrar.php | 6 ++-- lib/Listener/DerivedStatusListener.php | 10 +++--- .../ProcessMining/DwellTimeAnalyzer.php | 2 +- lib/Service/ProcessMiningService.php | 2 +- .../Settings/SchemaAnnotationReconciler.php | 1 - .../Status/CaseStateFieldRuleProjector.php | 30 ++++++++--------- lib/Service/Status/DerivedStatusEvaluator.php | 18 +++++------ lib/Service/Status/DerivedStatusService.php | 14 ++++---- lib/Service/Status/StatusDeclaration.php | 12 +++---- lib/Service/Status/StatusDeclarations.php | 12 +++---- lib/Service/Status/StatusDwellService.php | 18 +++++------ lib/Service/Status/StatusDwellTimer.php | 12 +++---- .../Status/StatusFieldRuleDeclaration.php | 14 ++++---- lib/Service/StatusTransitionService.php | 2 +- lib/Service/WorkQueueService.php | 4 +-- .../case/CaseStatusDeclarationPanel.vue | 20 ++++++------ src/components/cells/DwellDaysCell.vue | 8 ++--- src/registry.js | 4 +-- src/services/cellWidgets.js | 2 +- src/services/schemaRules.js | 18 +++++------ src/utils/caseLifecycleHelpers.js | 1 - src/utils/statusDeclaration.js | 12 +++---- src/utils/statusFieldRules.js | 24 +++++++------- src/utils/statusTypeForm.js | 4 +-- .../settings/components/StatusTypeForm.vue | 32 +++++++++---------- src/views/settings/tabs/RulesTab.vue | 16 +++++----- src/views/settings/tabs/StatusesTab.vue | 4 +-- tests/Support/MakesStatusDeclarations.php | 4 +-- .../ProcessMining/DwellTimeAnalyzerTest.php | 6 ++-- .../Service/Status/CaseDwellFieldsTest.php | 16 +++++----- .../CaseStateFieldRuleProjectorTest.php | 4 +-- .../Unit/Service/Status/DerivedStatusTest.php | 16 +++++----- .../Service/Status/StatusDwellBreachTest.php | 12 +++---- .../Service/Status/StatusDwellTimerTest.php | 14 ++++---- .../Status/StatusFieldRuleDeclarationTest.php | 4 +-- .../Service/Status/StatusWaitingOnTest.php | 12 +++---- .../Status/WorkQueueWaitingCountTest.php | 10 +++--- tests/vitest/caseDerivedStatusReasons.spec.js | 2 +- tests/vitest/caseFieldRulesPanel.spec.js | 2 +- tests/vitest/caseListDwellColumn.spec.js | 2 +- tests/vitest/caseTypeRules.spec.js | 2 +- tests/vitest/statusFieldRules.spec.js | 2 +- 42 files changed, 204 insertions(+), 206 deletions(-) diff --git a/lib/AppInfo/Registrar/DerivedStatusListenerRegistrar.php b/lib/AppInfo/Registrar/DerivedStatusListenerRegistrar.php index 5cb9026359..f0207c7625 100644 --- a/lib/AppInfo/Registrar/DerivedStatusListenerRegistrar.php +++ b/lib/AppInfo/Registrar/DerivedStatusListenerRegistrar.php @@ -17,7 +17,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -36,7 +36,7 @@ * * @psalm-suppress UnusedClass * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class DerivedStatusListenerRegistrar { @@ -65,7 +65,7 @@ class DerivedStatusListenerRegistrar { * * @return void * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function register(IRegistrationContext $context): void { // ONE journal for the two listeners. An autowired class is built again diff --git a/lib/Listener/DerivedStatusListener.php b/lib/Listener/DerivedStatusListener.php index 3a2a039769..3c39b84eed 100644 --- a/lib/Listener/DerivedStatusListener.php +++ b/lib/Listener/DerivedStatusListener.php @@ -40,7 +40,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -63,7 +63,7 @@ * * @implements IEventListener * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class DerivedStatusListener implements IEventListener { @@ -115,7 +115,7 @@ public function __construct( * * @return void * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function handle(Event $event): void { if ($event instanceof ObjectUpdatingEvent === false) { @@ -171,7 +171,7 @@ public function handle(Event $event): void { * * @return array|null The payload, or null. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function payload(ObjectEntity $entity): ?array { try { @@ -191,7 +191,7 @@ private function payload(ObjectEntity $entity): ?array { * * @return bool True when this is a case. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function isCaseSchema(array $object): bool { $expected = $this->settingsService->getConfigValue('case_schema'); diff --git a/lib/Service/ProcessMining/DwellTimeAnalyzer.php b/lib/Service/ProcessMining/DwellTimeAnalyzer.php index 49fd08adf3..9ed3fe7bae 100644 --- a/lib/Service/ProcessMining/DwellTimeAnalyzer.php +++ b/lib/Service/ProcessMining/DwellTimeAnalyzer.php @@ -112,7 +112,7 @@ public function clock(): string { * * @return array Working days, keyed by statusType id. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function heldTotalsByStatus(array $casesById, ?DateTimeImmutable $now = null): array { if ($this->held === null) { diff --git a/lib/Service/ProcessMiningService.php b/lib/Service/ProcessMiningService.php index 3cda135779..566b123a00 100644 --- a/lib/Service/ProcessMiningService.php +++ b/lib/Service/ProcessMiningService.php @@ -92,7 +92,7 @@ public function __construct( * @return array The structured response body. * * @spec openspec/changes/process-mining-bottlenecks/tasks.md#T01 - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function getReport(array $params): array { $to = ($this->dates->tryParse($params['to'] ?? null) ?? $this->dates->today()); diff --git a/lib/Service/Settings/SchemaAnnotationReconciler.php b/lib/Service/Settings/SchemaAnnotationReconciler.php index cf08eace7f..d6cf1059bb 100644 --- a/lib/Service/Settings/SchemaAnnotationReconciler.php +++ b/lib/Service/Settings/SchemaAnnotationReconciler.php @@ -215,7 +215,6 @@ private function reconcileSchemaAnnotationBlocks(object $schemaMapper, int|strin * on a case-type store it never reads. * * @spec openspec/specs/status-transition-engine/spec.md - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md */ private function mergeOntoLiveSchema(object $schemaMapper, string $slug, array $annotations): int { // 🔴 RESOLVE INSIDE OUR OWN REGISTER. An unscoped slug lookup here merged diff --git a/lib/Service/Status/CaseStateFieldRuleProjector.php b/lib/Service/Status/CaseStateFieldRuleProjector.php index e802cd9623..cd63179b23 100644 --- a/lib/Service/Status/CaseStateFieldRuleProjector.php +++ b/lib/Service/Status/CaseStateFieldRuleProjector.php @@ -38,7 +38,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -55,7 +55,7 @@ /** * Projects a case type's per-status field rules onto the live case schema. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class CaseStateFieldRuleProjector { @@ -133,7 +133,7 @@ public function __construct( * * @return array> The states block. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function statesOf(string $caseTypeId): array { $statusTypes = $this->store->rowsOfType( @@ -183,7 +183,7 @@ public function statesOf(string $caseTypeId): array { * * @return bool True when the schema configuration was rewritten. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function publish(string $caseTypeId): bool { if ($caseTypeId === '') { @@ -227,7 +227,7 @@ public function publish(string $caseTypeId): bool { * * @return array The lifecycle block. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function lifecycleOf(array $configuration): array { $lifecycle = ($configuration[self::LIFECYCLE_KEY] ?? []); @@ -245,7 +245,7 @@ private function lifecycleOf(array $configuration): array { * * @return array The states. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function statesIn(array $lifecycle): array { $states = ($lifecycle['states'] ?? []); @@ -268,7 +268,7 @@ private function statesIn(array $lifecycle): array { * * @return array The lifecycle block. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function withStates(array $lifecycle, array $states): array { unset($lifecycle['states']); @@ -289,7 +289,7 @@ private function withStates(array $lifecycle, array $states): array { * * @return object|null The schema entity. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function liveCaseSchema(): ?object { try { @@ -318,7 +318,7 @@ private function liveCaseSchema(): ?object { * * @return array The configuration. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function configurationOf(object $schema): array { $configuration = ($schema->getConfiguration() ?? []); @@ -342,7 +342,7 @@ private function configurationOf(object $schema): array { * * @return bool True when the schema was written. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function write(object $schema, array $configuration, string $caseTypeId): bool { if ($this->schemaMapper === null) { @@ -387,7 +387,7 @@ private function write(object $schema, array $configuration, string $caseTypeId) * * @return mixed The block to write. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public static function carryForwardStates(string $annotationKey, mixed $declared, mixed $live): mixed { if ($annotationKey !== self::LIFECYCLE_KEY || is_array($declared) === false) { @@ -422,7 +422,7 @@ public static function carryForwardStates(string $annotationKey, mixed $declared * * @return array The statusType uuids. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function stateKeysOf(string $caseTypeId): array { $keys = []; @@ -448,7 +448,7 @@ private function stateKeysOf(string $caseTypeId): array { * * @return array> Field names keyed by statusType uuid. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function requiredByStatus(string $caseTypeId): array { $properties = $this->store->rowsOfType( @@ -486,7 +486,7 @@ private function requiredByStatus(string $caseTypeId): array { * * @return array>> The block. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function foldInRequired(array $fields, array $required): array { if ($required === []) { @@ -524,7 +524,7 @@ private function foldInRequired(array $fields, array $required): array { * * @return array The merged states. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function mergeStates(array $live, array $own, array $ownKeys): array { $merged = $live; diff --git a/lib/Service/Status/DerivedStatusEvaluator.php b/lib/Service/Status/DerivedStatusEvaluator.php index 9fe2bf9537..449eb21bab 100644 --- a/lib/Service/Status/DerivedStatusEvaluator.php +++ b/lib/Service/Status/DerivedStatusEvaluator.php @@ -27,7 +27,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -37,7 +37,7 @@ /** * Evaluates a status's declared conditions against a case. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class DerivedStatusEvaluator { @@ -84,7 +84,7 @@ public function __construct( * `satisfied` whether every condition holds, and `unmet` names what * is missing, in declaration order. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function evaluate(array $statusType, array $case): array { $conditions = $this->declaration->derivedWhen(statusType: $statusType); @@ -112,7 +112,7 @@ public function evaluate(array $statusType, array $case): array { * * @return bool * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function holds(array $condition, array $case): bool { $kind = (string)($condition['kind'] ?? ''); @@ -148,7 +148,7 @@ private function holds(array $condition, array $case): bool { * * @return bool * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function hasDocumentOfType(array $case, string $documentType): bool { if ($documentType === '') { @@ -191,7 +191,7 @@ private function hasDocumentOfType(array $case, string $documentType): bool { * * @return mixed The value, or null when the path resolves to nothing. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function read(array $case, string $path): mixed { $cursor = $case; @@ -218,7 +218,7 @@ private function read(array $case, string $path): mixed { * * @return bool * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function isPresent(mixed $value): bool { if ($value === null || $value === false) { @@ -243,7 +243,7 @@ private function isPresent(mixed $value): bool { * * @return string * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function scalar(mixed $value): string { if (is_bool($value) === true) { @@ -273,7 +273,7 @@ private function scalar(mixed $value): string { * * @return string * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function reasonFor(array $condition): string { $label = trim((string)($condition['label'] ?? '')); diff --git a/lib/Service/Status/DerivedStatusService.php b/lib/Service/Status/DerivedStatusService.php index 7283e0b049..caaa19e6c7 100644 --- a/lib/Service/Status/DerivedStatusService.php +++ b/lib/Service/Status/DerivedStatusService.php @@ -29,7 +29,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -42,7 +42,7 @@ /** * Resolves the derived status of a case and the reasons a derivation has not fired. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class DerivedStatusService { @@ -72,7 +72,7 @@ public function __construct( * * @return array}> * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function verdictsFor(array $case): array { $caseTypeId = (string)($case['caseType'] ?? ''); @@ -121,7 +121,7 @@ public function verdictsFor(array $case): array { * * @return bool * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function isDerivedStatus(string $caseTypeId, string $statusTypeId): bool { if ($caseTypeId === '' || $statusTypeId === '') { @@ -161,7 +161,7 @@ public function isDerivedStatus(string $caseTypeId, string $statusTypeId): bool * * @return string|null The statusType id to move to, or null for no move. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function statusFor(array $case): ?string { $currentId = (string)($case['status'] ?? ''); @@ -195,7 +195,7 @@ public function statusFor(array $case): ?string { * * @return array{statusId: string, name: string, unmet: array}|null * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function blockingReasonFor(array $case): ?array { $currentOrder = $this->orderOf(case: $case, statusId: (string)($case['status'] ?? '')); @@ -223,7 +223,7 @@ public function blockingReasonFor(array $case): ?array { * * @return int The order, or 0 when the status does not resolve. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function orderOf(array $case, string $statusId): int { if ($statusId === '') { diff --git a/lib/Service/Status/StatusDeclaration.php b/lib/Service/Status/StatusDeclaration.php index f579351ccb..fe30fa5392 100644 --- a/lib/Service/Status/StatusDeclaration.php +++ b/lib/Service/Status/StatusDeclaration.php @@ -27,7 +27,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -37,7 +37,7 @@ /** * Reads the three declarations a statusType may carry. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class StatusDeclaration { @@ -106,7 +106,7 @@ class StatusDeclaration { * * @return array> The conditions worth evaluating. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function derivedWhen(array $statusType): array { $declared = ($statusType['derivedWhen'] ?? null); @@ -148,7 +148,7 @@ public function derivedWhen(array $statusType): array { * * @return bool True when the status declares at least one condition. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function isDerived(array $statusType): bool { return $this->derivedWhen(statusType: $statusType) !== []; @@ -161,7 +161,7 @@ public function isDerived(array $statusType): bool { * * @return string One of the three values; `us` when the status declares nothing. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function waitingOn(array $statusType): string { $declared = trim((string)($statusType['waitingOn'] ?? '')); @@ -179,7 +179,7 @@ public function waitingOn(array $statusType): string { * * @return int|null The maximum, or null when the status declares none. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function maximumDwell(array $statusType): ?int { $declared = ($statusType['maximumDwell'] ?? null); diff --git a/lib/Service/Status/StatusDeclarations.php b/lib/Service/Status/StatusDeclarations.php index f8edd79a72..1be396e5ff 100644 --- a/lib/Service/Status/StatusDeclarations.php +++ b/lib/Service/Status/StatusDeclarations.php @@ -27,7 +27,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -41,7 +41,7 @@ /** * The declarations of the status a case is in, and the dwell bookkeeping. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class StatusDeclarations { @@ -82,7 +82,7 @@ public function __construct( * derivation: array{statusId: string, name: string, unmet: array}|null * } * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function panelFor(array $case, ?DateTimeImmutable $now = null): array { $statusType = $this->lookup->rowFor(statusTypeId: (string)($case['status'] ?? '')); @@ -106,7 +106,7 @@ public function panelFor(array $case, ?DateTimeImmutable $now = null): array { * * @return bool * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function isDerivedStatus(string $caseTypeId, string $statusTypeId): bool { return $this->derived->isDerivedStatus(caseTypeId: $caseTypeId, statusTypeId: $statusTypeId); @@ -121,7 +121,7 @@ public function isDerivedStatus(string $caseTypeId, string $statusTypeId): bool * * @return array The payload. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function applyStatusChange(array $case, string $toStatus, ?DateTimeImmutable $now = null): array { return $this->dwell->applyStatusChange(case: $case, toStatus: $toStatus, now: $now); @@ -140,7 +140,7 @@ public function applyStatusChange(array $case, string $toStatus, ?DateTimeImmuta * * @return string|null The armed timer uuid, or null when nothing was armed. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function retime(string $caseId, string $toStatus): ?string { $this->timer->cancel(caseId: $caseId, reason: 'the case left the status this clock was about'); diff --git a/lib/Service/Status/StatusDwellService.php b/lib/Service/Status/StatusDwellService.php index 30a40e341d..690393bde9 100644 --- a/lib/Service/Status/StatusDwellService.php +++ b/lib/Service/Status/StatusDwellService.php @@ -40,7 +40,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ declare(strict_types=1); @@ -54,7 +54,7 @@ /** * Writes and reads the dwell numbers the case carries. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ class StatusDwellService { @@ -83,7 +83,7 @@ public function __construct( * * @return array The payload, with the dwell fields settled. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function applyStatusChange(array $case, string $toStatus, ?DateTimeImmutable $now = null): array { $moment = ($now ?? new DateTimeImmutable()); @@ -118,7 +118,7 @@ public function applyStatusChange(array $case, string $toStatus, ?DateTimeImmuta * * @return int Working days, never negative. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function elapsedWorkingDays(array $case, ?DateTimeImmutable $now = null): int { $entered = $this->enteredAt(case: $case); @@ -146,7 +146,7 @@ public function elapsedWorkingDays(array $case, ?DateTimeImmutable $now = null): * * @return bool False whenever the status declares no maximum. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function isBreached(array $case, ?int $maximum, ?DateTimeImmutable $now = null): bool { if ($maximum === null || $maximum < 1) { @@ -165,7 +165,7 @@ public function isBreached(array $case, ?int $maximum, ?DateTimeImmutable $now = * * @return array{days: int, maximum: int|null, breached: bool, enteredAt: string} * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function snapshot(array $case, ?int $maximum, ?DateTimeImmutable $now = null): array { $entered = $this->enteredAt(case: $case); @@ -195,7 +195,7 @@ public function snapshot(array $case, ?int $maximum, ?DateTimeImmutable $now = n * * @return array Working days, keyed by statusType id. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function totalsFor(array $case, ?DateTimeImmutable $now = null): array { $totals = []; @@ -230,7 +230,7 @@ public function totalsFor(array $case, ?DateTimeImmutable $now = null): array { * * @return DateTimeImmutable|null The moment, or null when nothing recorded one. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ private function enteredAt(array $case): ?DateTimeImmutable { $recorded = $this->dates->tryParse(value: ($case['currentStatusEnteredAt'] ?? null)); @@ -250,7 +250,7 @@ private function enteredAt(array $case): ?DateTimeImmutable { * * @return array * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ private function addTo(mixed $totals, string $statusTypeId, int $workingDays): array { $rebuilt = []; diff --git a/lib/Service/Status/StatusDwellTimer.php b/lib/Service/Status/StatusDwellTimer.php index 29777173a8..09f23f1377 100644 --- a/lib/Service/Status/StatusDwellTimer.php +++ b/lib/Service/Status/StatusDwellTimer.php @@ -31,7 +31,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -46,7 +46,7 @@ /** * Arms and cancels the engine timer behind a status's maximum dwell. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class StatusDwellTimer { @@ -102,7 +102,7 @@ public function __construct( * * @return string|null The armed timer uuid, or null when the engine is absent or refused. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function arm(string $caseId, string $statusTypeId, int $maximumDwell, string $statusName = ''): ?string { if ($caseId === '' || $maximumDwell < 1) { @@ -189,7 +189,7 @@ public function arm(string $caseId, string $statusTypeId, int $maximumDwell, str * * @return int How many timers were cancelled. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function cancel(string $caseId, string $reason = 'status changed'): int { if ($caseId === '') { @@ -220,7 +220,7 @@ public function cancel(string $caseId, string $reason = 'status changed'): int { * * @return object|null The FlowTimerService. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function engine(): ?object { return $this->settingsService->getOpenRegisterClass(TermijnTimerService::ENGINE_CLASS); @@ -239,7 +239,7 @@ private function engine(): ?object { * * @return void * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function degraded(string $operation, string $caseId, Throwable $error): void { $this->logger->warning( diff --git a/lib/Service/Status/StatusFieldRuleDeclaration.php b/lib/Service/Status/StatusFieldRuleDeclaration.php index 23511bdff0..55cde08a6a 100644 --- a/lib/Service/Status/StatusFieldRuleDeclaration.php +++ b/lib/Service/Status/StatusFieldRuleDeclaration.php @@ -34,7 +34,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -44,7 +44,7 @@ /** * Reads a status type's field rules and writes them in OpenRegister's shape. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class StatusFieldRuleDeclaration { @@ -99,7 +99,7 @@ class StatusFieldRuleDeclaration { * * @return array>> The `fields` block, possibly empty. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function fieldsBlock(array $statusType): array { $declared = ($statusType['fieldRules'] ?? null); @@ -128,7 +128,7 @@ public function fieldsBlock(array $statusType): array { * * @return array{0: string, 1: array}|null The kind and the entry. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function entryOf(mixed $rule): ?array { if (is_array($rule) === false) { @@ -176,7 +176,7 @@ private function entryOf(mixed $rule): ?array { * * @return array The group names. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function groupsOf(array $rule): array { $declared = ($rule['groups'] ?? null); @@ -212,7 +212,7 @@ private function groupsOf(array $rule): array { * * @return array|null The JSONLogic node. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function conditionOf(mixed $condition): ?array { if (is_array($condition) === false) { @@ -248,7 +248,7 @@ private function conditionOf(mixed $condition): ?array { * * @return array>> The block, ordered. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function inPublishedOrder(array $block): array { $ordered = []; diff --git a/lib/Service/StatusTransitionService.php b/lib/Service/StatusTransitionService.php index 61ac6b95ec..ce0d5623b2 100644 --- a/lib/Service/StatusTransitionService.php +++ b/lib/Service/StatusTransitionService.php @@ -373,7 +373,7 @@ private function actionOutcome(array $dispatched, string $caseId): array { * @throws RuntimeException When a closing transition carries no result type. * * @spec openspec/specs/status-transition-engine/spec.md - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ private function writeMove( array $caseAtSave, diff --git a/lib/Service/WorkQueueService.php b/lib/Service/WorkQueueService.php index 0b3724adca..364206d458 100644 --- a/lib/Service/WorkQueueService.php +++ b/lib/Service/WorkQueueService.php @@ -197,7 +197,7 @@ static function (array $a, array $b): int { * * @return array{ours: int, applicant: int, thirdParty: int, total: int} * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function countByWaitingOn(?string $userId = null): array { $objectService = $this->settingsService->getObjectService(); @@ -243,7 +243,7 @@ public function countByWaitingOn(?string $userId = null): array { * * @return array{ours: int, applicant: int, thirdParty: int, total: int} * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function tallyWaitingOn(array $cases): array { $counts = ['ours' => 0, 'applicant' => 0, 'thirdParty' => 0, 'total' => 0]; diff --git a/src/components/case/CaseStatusDeclarationPanel.vue b/src/components/case/CaseStatusDeclarationPanel.vue index 136d7515bd..67014b55fc 100644 --- a/src/components/case/CaseStatusDeclarationPanel.vue +++ b/src/components/case/CaseStatusDeclarationPanel.vue @@ -25,7 +25,7 @@ its status maximum, with no derivation pending, renders nothing at all rather than three empty lines. - @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + @spec openspec/specs/status-transition-engine/spec.md -->