Skip to content

fix(reviews): declare auteur and status, so they are not dropped on save - #923

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/reviews-lose-their-author-and-status
Sep 1, 2026
Merged

fix(reviews): declare auteur and status, so they are not dropped on save#923
rubenvdlinde merged 1 commit into
developmentfrom
fix/reviews-lose-their-author-and-status

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Two catalog-ratings specs and one suite-wizard spec, all red on development. The first two share one cause, and it is a real data-loss bug, not a test problem.

Reviews were stored with no author and no moderation status

reviews index: expected /admin/i, received "Review columns …—6—"
reviews: expected status "pending", received ""

ReviewService does set both — line 231 stamps auteur from the authenticated session, line 232 forces status to STATUS_PENDING, and the file docblock explains at length that a client-supplied value is stripped and re-derived.

The schema never declared either property. software-review in lib/Settings/softwarecatalogus_register.json had eight properties and neither auteur nor status was among them, so OpenRegister discarded both on every save — silently. The write succeeds, the object comes back, the two fields are simply not there.

The em-dash in the failing row is the bug rendered: that column had nothing to show.

Why this matters beyond an empty column

The public RBAC read rule only ever matches status approved, and ModerationService transitions a review out of pending. With status unstored, a review has no state to moderate, so the whole moderation path was inert.

The mock register had both all along, which is why unit tests never caught it. The two definitions are now byte-identical — checked.

suite-wizard asserted a hash URL against a path route

Expected pattern: /#\/suites\/[^/]+$/
Received string:  ".../apps/stackiq/suites/d2f67fa0-…"

The shell navigated to the correct detail page for the newly created suite. Only the # was missing, so the pattern waited the full 30s while the browser sat on the right page. It now accepts either shape; the uuid is what proves the suite was created.

Two catalog-ratings specs and one suite-wizard spec, all red on development.
The first two share one cause and it is a real data-loss bug, not a test
problem.

## Reviews were stored with no author and no moderation status

  reviews index: expected /admin/i, received "Review columns …—6—"
  reviews: expected status "pending", received ""

ReviewService does set both. Line 231 stamps `auteur` from the authenticated
session, line 232 forces `status` to STATUS_PENDING, and the docblock at the
top of the file explains at length that a client-supplied value is stripped
and re-derived.

The schema never declared either property. `software-review` in
lib/Settings/softwarecatalogus_register.json had eight properties and neither
`auteur` nor `status` was among them, so OpenRegister discarded both on every
save. Silently: the write succeeds, the object comes back, the two fields are
simply not there.

The em-dash in the failing row IS the bug rendered. That column had nothing
to show.

⚠️ This is worse than an empty column. The public read rule only ever matches
status approved, and ModerationService transitions a review out of
pending. With `status` unstored, a review has no state to moderate, so the
whole moderation path was inert.

The mock register HAD both all along, which is why this never showed up in
unit tests. The two definitions are now byte-identical, checked.

## suite-wizard asserted a hash URL against a path route

  Expected pattern: /#\\/suites\\/[^/]+$/
  Received string:  ".../apps/stackiq/suites/d2f67fa0-…"

The shell navigated to the correct detail page for the newly created suite.
Only the "#" was missing, so the pattern waited 30s while the browser sat on
the right page. It now accepts either shape; the uuid is what proves the
suite was created.
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/stackiq @ 42340f4

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
check-l10n-js
composer ✅ 130/130
npm ✅ 711/711
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-01 10:54 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 7a68361 into development Sep 1, 2026
50 checks passed
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