Skip to content

fix(transitions): every status transition 500s on an empty guard details - #1941

Merged
rubenvdlinde merged 4 commits into
developmentfrom
fix/every-status-transition-500s-on-an-empty-guard-details
Sep 8, 2026
Merged

fix(transitions): every status transition 500s on an empty guard details#1941
rubenvdlinde merged 4 commits into
developmentfrom
fix/every-status-transition-500s-on-an-empty-guard-details

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Making a status transition from the UI has been answering 500 since
73c0570e2.
The dialog never closes and the case does not move. This is a
user-facing break, not only three red E2E tests.

From nextcloud.log on the failing run, six times:

POST /index.php/apps/dossiq/api/case/<id>/transition
[dossiq] StatusTransitionController: execute failed
Property 'evaluatedGuards.0.details' expects object but got empty ({}).
For non-required object properties, set this to null to clear the field.

Why

GuardRegistry::evaluateAll() copies each GuardResult verbatim into the
snapshot that CaseStatusStore persists as statusRecord.evaluatedGuards,
whose details is declared type: object. GuardResult::$details defaults to
an empty array, and a guard with nothing to report leaves it that way, so the
rejected shape is the common one, not an edge case.

It stayed invisible until the status checklist guard began being appended to
every transition. Before that, a transition declaring no guards produced no
snapshot entries at all, so there was nothing to reject. Afterwards every
transition carried at least entry 0. StatusTransitionController::execute()
catches \Throwable, so a validation failure surfaced as a 500 rather than as
a message anyone could read, which is why this reads as a hung dialog.

The fix

Empty details is written as null, which is what OpenRegister's own error
message asks for. Details a guard does populate are untouched: the
unknown-guard branch and every failing guard keep what they reported, or the
reason a transition was refused would stop being recorded. There is a control
test for that.

The third test states the invariant the schema actually cares about, that no
snapshot entry carries an empty details, so a new guard cannot reintroduce
this by defaulting the field again.

Verification

PHPCS on lib                                     clean
tests/Unit/Service/Transitions (125 tests)       OK, 255 assertions
the three new tests, with the fix reverted       2 of 3 fail

One trap worth recording

This app's vendor autoloader maps OCA\Dossiq\ to $baseDir/lib, where
$baseDir is whichever checkout composer installed in. A first run of these
tests silently exercised a sibling checkout's code and reported the fix as
ineffective. The numbers above come from a run that loads this branch's lib/,
confirmed by reflecting on the loaded class's file path before running.

Making a status transition from the UI has been answering 500 since 73c0570.
The dialog never closes and the case does not move. This is a user-facing
break, not only a test failure.

    POST /apps/dossiq/api/case/<id>/transition
    [dossiq] StatusTransitionController: execute failed
    Property 'evaluatedGuards.0.details' expects object but got empty ({}).
    For non-required object properties, set this to null to clear the field.

GuardRegistry copies each GuardResult verbatim into the snapshot that is
persisted as statusRecord.evaluatedGuards, whose details is declared type
object. GuardResult::$details defaults to an empty array, and a guard with
nothing to report leaves it that way, so the common case is the rejected one.

It stayed invisible until the status checklist guard began being appended to
every transition. Before that, a transition declaring no guards produced no
snapshot entries at all, so there was nothing to reject. Afterwards every
transition carried at least entry 0, and StatusTransitionController::execute
catches Throwable, so the validation failure surfaced as a 500 rather than as a
message anyone could read.

Empty details is now written as null, which is what OpenRegister's own error
message asks for. Details that a guard does populate are untouched: the
unknown-guard branch and every failing guard keep what they reported, or the
reason a transition was refused would stop being recorded.

Three tests. Two fail without the change and pass with it. The third states the
invariant the schema cares about, that no snapshot entry carries an empty
details, so a new guard cannot reintroduce this by defaulting the field again.

Verified locally: PHPCS clean on lib, and 125 tests in
tests/Unit/Service/Transitions pass. One trap worth recording: this app's
vendor autoloader resolves OCA\Dossiq to whatever checkout composer installed
in, so a first run of these tests silently exercised a sibling checkout's code
and reported the fix as ineffective. The run above loads this branch's lib.
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/dossiq @ 174244d

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue3-compile
test-l10n
format
check-schema-l10n
check-l10n-js
composer ✅ 106/106
npm ✅ 541/541
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates

Quality workflow — 2026-09-08 13:51 UTC

Download the full PDF report from the workflow artifacts.

Conduction Release Bot added 3 commits September 8, 2026 15:59
…us-transition-500s-on-an-empty-guard-details
CI reported three PHPUnit warnings on this file:

    "@uses set" is invalid

The class docblock opened a paragraph with the uses tag followed by the word
"set", to explain why the coverage-metadata list matches GuardDialectTest's.
PHPUnit read that as an annotation carrying the value "set" and rejected it,
on all three tests, and the parser does not care that the tag sat mid-sentence
rather than at the start of a line.

The paragraph now says the same thing without writing a tag name at all, and
records the trap for the next person, since the explanation is worth keeping
and was the thing that broke.

The only at-sign left in the file outside the annotation block is the SPDX
copyright email. Verified by listing every at-sign in the file, and by running
with warnings displayed rather than filtered: 3 tests, 10 assertions, OK, no
warnings.
…us-transition-500s-on-an-empty-guard-details
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/dossiq @ 2526537

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue3-compile
test-l10n
format
check-schema-l10n
check-l10n-js
composer ✅ 106/106
npm ✅ 541/541
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates

Quality workflow — 2026-09-08 14:21 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 384e862 into development Sep 8, 2026
46 of 48 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/every-status-transition-500s-on-an-empty-guard-details branch September 8, 2026 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant