fix(decidiq): contract delegation was silently off — the event namespace moved - #728
Merged
Merged
Conversation
…ace moved `ContractApprovalService` pinned `\OCA\Decidesk\Event\DecisionRequestedEvent`. The decision app renamed to `OCA\Decidiq` with no compatibility alias, so `isDelegationConfigured()` began returning false on instances where the app was installed, and contract approvals stopped delegating. Nothing reported it, and nothing could: `class_exists` going false is exactly what an uninstalled optional app looks like, which is the case the guard was written for. The feature reads as "not configured" rather than "broken". Measured on a running instance: OCA\Decidesk\Event\DecisionRequestedEvent MISSING OCA\Decidiq\Event\DecisionRequestedEvent EXISTS The constant is now a LIST, newest first, resolved to the first that exists. The old spelling stays until no supported install ships it — dropping it would break the integration in the other direction during a staggered upgrade, which is the window this broke in to begin with. An app cannot move another app's class name; it can only follow it. So the test asserts that property rather than a literal: the list names the current namespace, tries it first, and still names the old one. Pinning one spelling fails a test now instead of silently disabling a feature later. NOT changed: `SOURCE_APP = 'softwarecatalog'`. That is this app's id AS THE DECISION APP KNOWS IT and is echoed back on the conclusion event, so it moves only when both sides move together. Found by a fleet sweep for stale cross-app namespaces after the same defect was confirmed in dossiq; openregister (→ Keepiq) and decidiq (→ Filinq) carried it too and are fixed in their own repos.
Psalm rejected the previous commit: `Type null cannot be called as a class` at `new $eventClass(...)`. It was right, and the shape it caught is worth naming. The method called `isDelegationConfigured()` — which resolves the class and throws away the answer — and then resolved AGAIN at the call site with no guard. Two lookups of the same thing, only the first of them checked. The second had a `?string` reaching `new`, so an instance with no decision app installed would have gone from a clear "delegation is not available" to a fatal on a null class name. Now it resolves once, narrows, and fails closed on the null — the same refusal as before, from the value actually used.⚠️ I did not run psalm locally before pushing; I ran phpunit, phpcs and phpstan, and phpstan does not flag this. One tool green is not the gate green, and the tools disagree by design. psalm "No errors found", phpcs 0, phpstan [OK] on the touched file.
Contributor
Quality Report — ConductionNL/stackiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ❌ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue-demi | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 130/130 | |||
| npm | ✅ | ✅ 720/720 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-24 10:08 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/stackiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue-demi | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 130/130 | |||
| npm | ✅ | ✅ 720/720 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-24 10:52 UTC
Download the full PDF report from the workflow artifacts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ContractApprovalServicepinned\OCA\Decidesk\Event\DecisionRequestedEvent. The decision app renamed toOCA\Decidiqwith no compatibility alias, soisDelegationConfigured()returned false on instances where the app was installed, and contract approvals stopped delegating.Nothing reported it, and nothing could.
class_existsgoing false is exactly what an uninstalled optional app looks like — the case the guard exists for. The feature reads as "not configured", never as broken.Measured on a running instance:
The fix
The constant becomes a list, newest first, resolved to the first that exists. The old spelling stays until no supported install ships it — dropping it breaks the integration in the other direction during a staggered upgrade, which is the window this broke in.
An app cannot move another app's class name; it can only follow it. So the new test asserts that property rather than a literal: the list names the current namespace, tries it first, and still names the old one.
Not changed:
SOURCE_APP = 'softwarecatalog'— this app's id as the decision app knows it, echoed back on the conclusion event. It moves only when both sides move together.Fleet context
Found by a sweep after the same defect was confirmed in dossiq. Three other apps carried it, each fixed in its own repo:
OCA\Decidesk\Event\*OCA\Decidesk\Event\*OCA\Doriath\Service\*OCA\Docudesk\Service\*phpunitbootstrap in this repo fails on an unrelated pre-existing error (OCA\Buildiq\Service\AppNavigationServicecannot resolveObjectServiceduringOC_App::loadApps()), confirmed identical before this change. phpcs and phpstan are clean on the touched file.🤖 Generated with Claude Code