Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 48 additions & 8 deletions lib/Service/ContractApprovalService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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<int, string>
*/
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
Expand Down Expand Up @@ -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.
*
Expand All @@ -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);
Expand All @@ -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,
Expand Down
35 changes: 31 additions & 4 deletions tests/Unit/Service/ContractApprovalServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading