diff --git a/lib/Service/ContractApprovalService.php b/lib/Service/ContractApprovalService.php index f8388c27..87d8ac76 100644 --- a/lib/Service/ContractApprovalService.php +++ b/lib/Service/ContractApprovalService.php @@ -57,10 +57,24 @@ */ class ContractApprovalService { /** - * The fully-qualified decidesk request-event class — the in-process - * existence guard (decidesk installed + autoloaded) for delegation. + * The fully-qualified request-event class spellings, NEWEST FIRST — the + * in-process existence guard (the decision app installed + autoloaded) for + * delegation. + * + * TWO SPELLINGS because a cross-app event class name is a RUNTIME lookup + * this app can only follow, never move. The decision app renamed from + * OCA\Decidesk to OCA\Decidiq with no compatibility alias, and this constant + * named only the old one — so `isDelegationConfigured()` returned false on an + * instance where the app was installed, and contract approvals silently + * stopped delegating. Measured: OCA\Decidiq\Event\DecisionRequestedEvent + * EXISTS, OCA\Decidesk\Event\DecisionRequestedEvent MISSING. + * + * @var array */ - public const DECISION_REQUESTED_EVENT = '\\OCA\\Decidesk\\Event\\DecisionRequestedEvent'; + public const DECISION_REQUESTED_EVENTS = [ + '\\OCA\\Decidiq\\Event\\DecisionRequestedEvent', + '\\OCA\\Decidesk\\Event\\DecisionRequestedEvent', + ]; /** * This consumer app id, stamped on the request event as `sourceApp` and @@ -163,9 +177,28 @@ public function __construct( * @spec openspec/specs/contract-decision-delegation/spec.md */ public function isDelegationConfigured(): bool { - return class_exists(self::DECISION_REQUESTED_EVENT); + return ($this->resolveRequestEventClass() !== null); }//end isDelegationConfigured() + /** + * The first request-event class that actually exists, or null. + * + * Resolving rather than assuming: this app can only follow the decision + * app's namespace, and a hard-coded spelling turns a rename over there into + * a silently disabled feature over here. + * + * @return string|null The event FQN, or null when the decision app is absent. + */ + private function resolveRequestEventClass(): ?string { + foreach (self::DECISION_REQUESTED_EVENTS as $candidate) { + if (class_exists($candidate) === true) { + return $candidate; + } + } + + return null; + }//end resolveRequestEventClass() + /** * Raise a decidesk Decision for a contract approval or renewal. * @@ -192,9 +225,17 @@ public function isDelegationConfigured(): bool { * It is a payload discriminator, not a second responsibility. */ public function submitForApproval(string $contractUuid, bool $isRenewal = false): string { - if ($this->isDelegationConfigured() === false) { - // Fail closed — never auto-approve when decidesk is not installed. - throw new RuntimeException('Contract approval delegation is not available (decidesk event contract not installed).'); + // Resolve ONCE and narrow here, rather than calling + // isDelegationConfigured() and resolving again below. Two lookups of the + // same thing can disagree — and the second one has no guard, so a null + // would reach `new $eventClass(...)` as a call on null. Psalm caught + // exactly that shape. + $eventClass = $this->resolveRequestEventClass(); + if ($eventClass === null) { + // Fail closed — never auto-approve when the decision app is absent. + throw new RuntimeException( + 'Contract approval delegation is not available (decision event contract not installed).' + ); } $contract = $this->loadContract(contractUuid: $contractUuid); @@ -209,7 +250,6 @@ public function submitForApproval(string $contractUuid, bool $isRenewal = false) $decisionType = self::DECISION_TYPE_RENEWAL; } - $eventClass = self::DECISION_REQUESTED_EVENT; $event = new $eventClass( self::SOURCE_APP, self::SUBJECT_REGISTER, diff --git a/tests/Unit/Service/ContractApprovalServiceTest.php b/tests/Unit/Service/ContractApprovalServiceTest.php index 4ad9e0ab..47990c58 100644 --- a/tests/Unit/Service/ContractApprovalServiceTest.php +++ b/tests/Unit/Service/ContractApprovalServiceTest.php @@ -80,18 +80,45 @@ private function makeService(): ContractApprovalService { }//end makeService() /** - * Delegation is NOT configured when decidesk's event class is absent. + * Delegation is NOT configured when NO spelling of the event class exists. * - * The decidesk app is not installed in the unit-test autoload scope, so the - * class_exists guard returns false — delegation is reported unconfigured. + * The decision app is not installed in the unit-test autoload scope, so + * every candidate misses and delegation is reported unconfigured. * * @return void */ public function testDelegationNotConfiguredWhenDecideskAbsent(): void { - $this->assertFalse(class_exists(ContractApprovalService::DECISION_REQUESTED_EVENT)); + foreach (ContractApprovalService::DECISION_REQUESTED_EVENTS as $candidate) { + $this->assertFalse(class_exists($candidate)); + } + $this->assertFalse($this->makeService()->isDelegationConfigured()); }//end testDelegationNotConfiguredWhenDecideskAbsent() + /** + * The candidate list names the CURRENT namespace and still names the old one. + * + * Pinning one spelling is what broke this: the decision app renamed to + * OCA\Decidiq without an alias, `isDelegationConfigured()` went false on an + * instance where the app was installed, and contract approvals silently + * stopped delegating. An app cannot move another app's class name — it can + * only follow it — so the property worth holding is that this LISTS the + * spellings, newest first, rather than pinning one. + * + * @return void + */ + public function testTheEventCandidatesFollowTheRename(): void { + $candidates = ContractApprovalService::DECISION_REQUESTED_EVENTS; + + $this->assertGreaterThanOrEqual(2, count($candidates)); + $this->assertStringContainsString('Decidiq', $candidates[0], 'The current namespace must be tried first.'); + $this->assertStringContainsString( + 'Decidesk', + implode(' ', $candidates), + 'The old namespace must stay listed until no supported install ships it.' + ); + }//end testTheEventCandidatesFollowTheRename() + /** * Submitting fails CLOSED (throws) when decidesk's event class is absent — * the contract is never advanced and status is never set to Actief, and the