fix(setup): forbidden must empty the unmet lists — completed alone is inert - #576
Merged
Merged
Conversation
… inert Follow-up to #575, which did not work. Caught by live verification, not by the tests, because the tests asserted the wrong thing. #575 short-circuited `completed` when the server answers 401/403. CnAppRoot never reads `completed`. It gates the blocking wizard on requiredUnmet.length > 0 and the non-gating auto-open on requiredUnmet.length === 0 && optionalUnmet.length > 0 so the fix changed nothing a user could see. Verified against a live instance after publishing 2.1.0-vue3.14 and bumping openbuild to it: rbac-editor still landed on "Set up this app / Welcome to OpenBuild" with .openbuild-schema-list count 0. The bundle did contain the new code — this was a wrong gate, not a stale build. `forbidden` now empties BOTH lists. A caller who may not READ setup state has no unmet setup work OF THEIR OWN; reporting steps as unmet to them described work they could neither see nor do. The previous test actively asserted the broken behaviour — // requiredUnmet still reflects the (unknown) server state expect(s.requiredUnmet.value.map((x) => x.id)).toEqual(['region']) — which is why 10 green tests sat on top of a fix that did nothing. Replaced with the assertion that matches the real gate, plus coverage that the lists come BACK when a later fetch succeeds (or the wizard would stay hidden from someone who now genuinely needs it) and that a 500 still shows it to an admin.
rubenvdlinde
added a commit
to ConductionNL/openbuild
that referenced
this pull request
Aug 2, 2026
…ns can use the app (#87) Picks up ConductionNL/nextcloud-vue#576: a 401/403 from /api/setup/status now empties the unmet-steps lists, so a non-admin is no longer shown the first-time-setup wizard. Why this bump matters more than the version delta suggests: after #81 gave non-admins OpenRegister-level read on the schemas, an editor still could not use OpenBuild. They landed on "Welcome to OpenBuild / Set up this app" with .openbuild-schema-list count 0 — the setup endpoints are admin-only, they answer 403, and useSetupStatus read that as "setup unfinished". Also corrects a drift found on the way: node_modules held 2.1.0-vue3.7 while package.json and the lockfile both said 2.1.0-vue3.13. Nobody had run an install since that bump, so local builds were linking a library three versions behind the pin. This commit was built from a clean `npm ci`. New: tests/e2e/non-admin-access.spec.ts, a regression suite for the OUTCOME rather than any one layer. Two consecutive fixes for this looked green while the app stayed broken for the user — openbuild#76's grant, then a nc-vue fix that short-circuited `completed`, which CnAppRoot never reads. The only assertion that would have caught both is "a non-admin sees the app", so that is what these three tests assert: - an editor reaches the schema designer AND no setup wizard (both halves — asserting the wizard's absence alone passes on a blank page) - an editor sees the app they were granted - an outsider sees none — the control proving openbuild's row-level filter still runs on top of OR's coarse `authenticated` read grant Verified live against the disposable instance: 3 passed. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
rubenvdlinde
added a commit
that referenced
this pull request
Aug 2, 2026
… inert (#577) Port of #576 to the beta line, where #574 has the same defect: it short-circuited `completed`, which CnAppRoot never reads. CnAppRoot gates the blocking wizard on `requiredUnmet.length > 0` (CnAppRoot.vue:2008) and the auto-open on `optionalUnmet.length > 0`, so the fix was inert here too. Proven on the vue3 line: after publishing the equivalent of #574 there and bumping openbuild to it, a non-admin STILL landed on "Set up this app" with the new code present in the bundle. Fixing the gate is what made it work. `forbidden` now empties BOTH lists. A caller who may not READ setup state has no unmet setup work OF THEIR OWN. The old test asserted the broken behaviour outright, which is why the suite stayed green over a fix that did nothing. Replaced with the assertion matching the real gate. 139 tests pass across useSetupStatus and CnAppRoot.
rubenvdlinde
pushed a commit
that referenced
this pull request
Aug 3, 2026
… `completed`
The setup wizard parked itself over the app on every fresh browser profile
even though `/api/setup/status` answered `200 {completed:true}` with every
step it reports marked done. That is the "the setup gate never clears"
report, and it is why ~98 pipelinq UI e2e specs died on selector timeouts:
Playwright always starts from a clean profile, so the localStorage dismissal
that hides this in day-to-day use is never present.
Root cause is a shape mismatch, not a status bug. An app's manifest may
declare optional steps its SetupController never emits a status key for —
pipelinq declares 7 steps and reports 4, with the optional `demo-data`
run-action reported nowhere. `useSetupStatus` maps a missing key to
`done: false`, so an UNREPORTED step is indistinguishable from an UNDONE one
and `optionalUnmet` is non-empty for ever. `optionalSetupGating` read only
those lists and never the server's own `completed` flag, so the auto-open
watcher fired on every mount, permanently.
Reproduced through a mounted CnAppRoot with pipelinq's real step list and the
verbatim payload its controller returns:
optionalUnmet ['demo-data'] optionalSetupGating true
setupWizardOpen true overlay rendered true
`completed` is the server's authoritative answer, so it now short-circuits
the offer. A genuinely incomplete setup still auto-opens — covered by a
positive control alongside, without which the new test could pass merely by
never rendering the overlay.
Control: reverting only the `optionalSetupGating` body turns the new test red
and leaves its positive control green; restoring turns it green. 8/8 in this
file, 171/171 across the 14 CnAppRoot + setup suites.
This is the second mechanism behind the same symptom. The first — a 403
gating non-admins out entirely — was fixed in #574/#575/#576/#577; case C of
the accompanying spec pins it so it cannot regress.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 3, 2026
…00 completed:true (#579) * test(app-root): cover the setup GATE through CnAppRoot, not just the composable Every existing first-time-setup test drove `useSetupStatus()` directly and awaited `refresh()` by hand. Not one mounted CnAppRoot, so not one could observe `phase` — and `phase` is the thing users hit. When the gate sticks there is no `<main>` and no nav, so every UI e2e spec in the consuming app dies on a selector timeout (~98 failures traced to it on pipelinq). That entire class of failure was invisible to this suite. Six cases through the real component: a positive control proving the gate is observable at all, the completed happy path, async manifest arrival, a 403, a second mount sharing the module-level cache, and a fresh manifest object identity. Case C is the one that mattered. `/api/setup/status` is admin-only (`#[AuthorizedAdminSetting]`), so it answers 200 {completed:true} to an admin and 403 to everyone else. Checking the endpoint in a browser as an admin therefore shows it perfectly healthy while the non-admin e2e user is gated out of the app entirely — two observations that look contradictory and are not. That is why this read as "gates even though status says completed". Control: replaying the file against `7b9639cb^` (pre-fix useSetupStatus) fails C and passes A/B/D/E, so the suite genuinely discriminates the behaviour that #574/#575/#576/#577 fixed rather than passing by construction. Restored to HEAD, all six pass. Tests only — no source change. * fix(app-root): an UNREPORTED setup step is not an UNDONE one — honour `completed` The setup wizard parked itself over the app on every fresh browser profile even though `/api/setup/status` answered `200 {completed:true}` with every step it reports marked done. That is the "the setup gate never clears" report, and it is why ~98 pipelinq UI e2e specs died on selector timeouts: Playwright always starts from a clean profile, so the localStorage dismissal that hides this in day-to-day use is never present. Root cause is a shape mismatch, not a status bug. An app's manifest may declare optional steps its SetupController never emits a status key for — pipelinq declares 7 steps and reports 4, with the optional `demo-data` run-action reported nowhere. `useSetupStatus` maps a missing key to `done: false`, so an UNREPORTED step is indistinguishable from an UNDONE one and `optionalUnmet` is non-empty for ever. `optionalSetupGating` read only those lists and never the server's own `completed` flag, so the auto-open watcher fired on every mount, permanently. Reproduced through a mounted CnAppRoot with pipelinq's real step list and the verbatim payload its controller returns: optionalUnmet ['demo-data'] optionalSetupGating true setupWizardOpen true overlay rendered true `completed` is the server's authoritative answer, so it now short-circuits the offer. A genuinely incomplete setup still auto-opens — covered by a positive control alongside, without which the new test could pass merely by never rendering the overlay. Control: reverting only the `optionalSetupGating` body turns the new test red and leaves its positive control green; restoring turns it green. 8/8 in this file, 171/171 across the 14 CnAppRoot + setup suites. This is the second mechanism behind the same symptom. The first — a 403 gating non-admins out entirely — was fixed in #574/#575/#576/#577; case C of the accompanying spec pins it so it cannot regress. --------- Co-authored-by: Ruben van der Linde <release-bot@conduction.nl>
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.
Follow-up to #575, which did not work.
What went wrong
#575 short-circuited
completedwhen the server answers 401/403.CnAppRootnever readscompleted. It gates the blocking wizard onand the non-gating auto-open on
So the fix changed nothing a user could see.
Caught by live verification, not by the tests: after publishing
2.1.0-vue3.14and bumping openbuild to it,rbac-editorstill landed on "Set up this app / Welcome to OpenBuild" with.openbuild-schema-listcount 0. The deployed bundle did contain the new code — this was a wrong gate, not a stale build.The fix
forbiddennow empties both unmet lists. A caller who may not read setup state has no unmet setup work of their own; reporting steps as unmet to them described work they could neither see nor do.Why 10 green tests didn't catch it
The test asserted the broken behaviour outright:
That comment was my assumption about which gate mattered, written without checking
CnAppRoot. It was wrong, and encoding it as an assertion is what made the suite agree with a fix that did nothing.Replaced with the assertion matching the real gate, plus:
556 tests pass across
tests/composablesandCnAppRoot.🤖 Generated with Claude Code