fix(rollback): rolling back restored NOTHING — it wrote to two fields the schema does not have - #94
Merged
Merged
Conversation
… the schema does not have
`onRollback` did:
obPatchApp({ manifest, version: `${v}-rollback-${hex}`, status: 'draft' })
`obPatchApp` PUTs the whole Application object at
/api/objects/openbuild/application/{uuid}. The `application` schema declares 15
properties and NEITHER `manifest` NOR `version` is one of them, so OpenRegister
dropped both. Only `status` survived — and since a rolled-back app is normally
already a draft, the entire feature was a visible no-op. Measured live, with a
control that changes a KNOWN property so "the write landed" is not assumed:
PUT description: 'CONTROL_MARKER_XYZ789' -> persisted (PUT landed)
version: 'PROBE-rollback-deadbeef' -> None (DROPPED)
manifest: {probeMarker: ...} -> None (DROPPED)
ApplicationManifestTab already hit this exact trap and moved to
PUT /api/applications/{slug}/manifest, leaving a comment saying so. The versions
tab was never updated. It now uses that same route; the endpoint round-trip is
verified independently ({"status":"ok","target":"version"}).
The `<version>-rollback-<hex>` label is deliberately NOT reinstated: there is no
`version` property to hold it, and inventing one is a schema change, not a bug
fix. `shortHex()` goes with it — the label was its only caller.
WHY THE SPEC COULD NOT SEE THIS, AND WHY IT WAS SKIPPED FOR SO LONG
The blocker note this file carried was wrong, and it was mine. It said the
sidebar "refuses to open — a UI defect to chase on its own" and that
`getByRole('tab', …)` "finds nothing — these are not ARIA tabs".
What was actually on screen was a MODAL. CnAppRoot offers the setup wizard
whenever every REQUIRED step is met but at least one OPTIONAL step is not
(`optionalSetupGating`, REQ-SETUP-NV-012) and opens it as a full `modal-mask`.
OpenBuild trips it permanently: its `store` step carries NO `required` key — the
word "optional" lives only in the title string — so `optionalUnmet` is never
empty. Users dismiss it once via localStorage; every Playwright context is
FRESH, so it reopened in every test. That mask sat over the sidebar toggle
(hence "visible but not actionable") and marked the background aria-hidden
(hence the missing tab role). One cause, both symptoms.
`document.elementsFromPoint(<centre of the toggle>)` named it in one probe,
after three wrong theories. New `suppressSetupWizard()` helper, applied to the
four specs that were exposed: this one, applicationDetailOverview,
non-admin-access, versionRouting.
THE SPEC NOW HAS TO BE ABLE TO FAIL
Two defects in my own test, both caught before shipping:
- Every seeded snapshot carries the same empty manifest, so "restored the
snapshot" and "did nothing" were byte-identical and the assertion could
never fail. It now plants a distinct manifest FIRST and asserts it is gone.
- The target was read as "first non-production row in the API response",
which is a DIFFERENT row than the first Roll back button on screen — so the
spec failed against a WORKING fix. It now reads the semver from the DOM row
that owns the button it clicks.
It also compares menu/pages only: the GET returns an EFFECTIVE manifest with
`runtime` injected and `name` added, so a whole-document match is impossible.
And it gets its OWN fixture (`pw-rollback`). It mutates the active manifest
twice per run, and versionRouting.spec.ts drives `pw-verchain` and reads
/manifest?_version= off it.
Three-way verified on the current spec: pre-fix FAILS (the planted manifest
survives the rollback), fixed PASSES 2/2.
Contributor
Quality Report — ConductionNL/openbuild @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 659/659 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Coverage: 89.4% (17/19 statements)
Quality workflow — 2026-08-02 16:45 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 2, 2026
…ed a field the schema does not have (#95) * fix(rollback): rolling back restored NOTHING — it wrote to two fields the schema does not have `onRollback` did: obPatchApp({ manifest, version: `${v}-rollback-${hex}`, status: 'draft' }) `obPatchApp` PUTs the whole Application object at /api/objects/openbuild/application/{uuid}. The `application` schema declares 15 properties and NEITHER `manifest` NOR `version` is one of them, so OpenRegister dropped both. Only `status` survived — and since a rolled-back app is normally already a draft, the entire feature was a visible no-op. Measured live, with a control that changes a KNOWN property so "the write landed" is not assumed: PUT description: 'CONTROL_MARKER_XYZ789' -> persisted (PUT landed) version: 'PROBE-rollback-deadbeef' -> None (DROPPED) manifest: {probeMarker: ...} -> None (DROPPED) ApplicationManifestTab already hit this exact trap and moved to PUT /api/applications/{slug}/manifest, leaving a comment saying so. The versions tab was never updated. It now uses that same route; the endpoint round-trip is verified independently ({"status":"ok","target":"version"}). The `<version>-rollback-<hex>` label is deliberately NOT reinstated: there is no `version` property to hold it, and inventing one is a schema change, not a bug fix. `shortHex()` goes with it — the label was its only caller. WHY THE SPEC COULD NOT SEE THIS, AND WHY IT WAS SKIPPED FOR SO LONG The blocker note this file carried was wrong, and it was mine. It said the sidebar "refuses to open — a UI defect to chase on its own" and that `getByRole('tab', …)` "finds nothing — these are not ARIA tabs". What was actually on screen was a MODAL. CnAppRoot offers the setup wizard whenever every REQUIRED step is met but at least one OPTIONAL step is not (`optionalSetupGating`, REQ-SETUP-NV-012) and opens it as a full `modal-mask`. OpenBuild trips it permanently: its `store` step carries NO `required` key — the word "optional" lives only in the title string — so `optionalUnmet` is never empty. Users dismiss it once via localStorage; every Playwright context is FRESH, so it reopened in every test. That mask sat over the sidebar toggle (hence "visible but not actionable") and marked the background aria-hidden (hence the missing tab role). One cause, both symptoms. `document.elementsFromPoint(<centre of the toggle>)` named it in one probe, after three wrong theories. New `suppressSetupWizard()` helper, applied to the four specs that were exposed: this one, applicationDetailOverview, non-admin-access, versionRouting. THE SPEC NOW HAS TO BE ABLE TO FAIL Two defects in my own test, both caught before shipping: - Every seeded snapshot carries the same empty manifest, so "restored the snapshot" and "did nothing" were byte-identical and the assertion could never fail. It now plants a distinct manifest FIRST and asserts it is gone. - The target was read as "first non-production row in the API response", which is a DIFFERENT row than the first Roll back button on screen — so the spec failed against a WORKING fix. It now reads the semver from the DOM row that owns the button it clicks. It also compares menu/pages only: the GET returns an EFFECTIVE manifest with `runtime` injected and `name` added, so a whole-document match is impossible. And it gets its OWN fixture (`pw-rollback`). It mutates the active manifest twice per run, and versionRouting.spec.ts drives `pw-verchain` and reads /manifest?_version= off it. Three-way verified on the current spec: pre-fix FAILS (the planted manifest survives the rollback), fixed PASSES 2/2. * fix(exports): the Exports tab was empty for every app — the filter used a field the schema does not have ExportJobsList fetched its rows with '?filter[applicationSlug]=' + this.applicationSlug which is wrong twice over, so the list was empty for every application, always: 1. `export-job` declares 18 properties and `applicationSlug` is NOT one of them — it is `applicationUuid`. Nothing ever wrote a slug onto these objects, so no stored row could carry one. 2. The `filter[...]` bracket syntax is not what the endpoint reads. Measured against the same 5 stored jobs: ?applicationUuid=<uuid> -> 1 (correct) ?filter[applicationUuid]=<u> -> 0 ?_filter[applicationUuid]=<u> -> 5 (ignored entirely) Same failure as the rollback fix in #94 and the VersionHistory fix before it: querying a field the schema does not declare. The code names it, so it looks real; only the schema says otherwise. `applicationSlug` is still correct for the SUBMIT endpoint (/api/applications/{slug}/exports), so both props are kept and ExportJobsTab now passes the uuid alongside it. Rows were also keyed `:key="job.uuid"`. `uuid` is not a property either, so EVERY key was undefined — identical keys let Vue reuse the wrong <tr> as statuses change under the 2s poll. Keyed on the OR object id instead. globalSetup: "already exists" is not the same as "usable" Role accounts were provisioned with POST /cloud/users and OCS 102 ("already exists") was accepted as the state we want. On a long-lived instance the account may PREDATE this harness and carry a different password: creation is skipped, every later login fails, and the specs run that role UNAUTHENTICATED — which reads as a product change rather than a fixture gap. Measured on the shared dev box: rbac-owner/editor/viewer authenticated 200, rbac-outsider 401, and versionRouting's "non-member must receive 404" failed with 401. globalSetup now PROVES the credentials work and repairs them when they do not. Live: "rbac-outsider existed with a different password — reset 200, now usable". NOT fixed here, and it blocks the ZIP round-trip spec: a submitted export never leaves `queued`. Driven by hand — submit 202, the queue entry is consumed, `occ background-job:execute` reports "Job executed!" — and the job object still reads status=queued with no errorMessage. OR ships Service\Lifecycle\TransitionEngine and no "TransitionEngine unavailable" warning is logged, so the transition runs and simply does not advance the job. That is a product blocker underneath export-zip.spec.ts, and a DIFFERENT one than the reason recorded in that file. --------- Co-authored-by: Ruben van der Linde <juan.claude@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
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.
The bug
obPatchAppPUTs the whole Application object. Theapplicationschema declares 15 properties and neithermanifestnorversionis one of them, so OpenRegister dropped both. Onlystatussurvived — and a rolled-back app is normally already a draft, so the entire feature was a visible no-op.Measured live, with a control that changes a known property so "the write landed" isn't assumed:
ApplicationManifestTabalready hit this exact trap and moved toPUT /api/applications/{slug}/manifest, leaving a comment saying so. The versions tab was never updated. It now uses that route; round-trip verified independently ({"status":"ok","target":"version"}).The
<version>-rollback-<hex>label is deliberately not reinstated — there's noversionproperty to hold it, and inventing one is a schema change, not a bug fix.shortHex()goes with it; the label was its only caller.Why the spec never caught it
The blocker note on this file was wrong, and it was mine. It claimed the sidebar "refuses to open — a UI defect to chase on its own" and that
getByRole('tab', …)"finds nothing — these are not ARIA tabs".It was a modal.
CnAppRootoffers the setup wizard whenever every required step is met but at least one optional one isn't (optionalSetupGating, REQ-SETUP-NV-012), as a fullmodal-mask. OpenBuild trips it permanently: itsstorestep carries norequiredkey — "optional" exists only in the title string — sooptionalUnmetis never empty. Users dismiss it once via localStorage; every Playwright context is fresh, so it reopened in every test.That mask sat over the sidebar toggle (→ "visible but not actionable") and marked the background
aria-hidden(→ missing tab role). One cause, both symptoms.document.elementsFromPoint(<centre of the toggle>)named it in one probe — after three wrong theories. NewsuppressSetupWizard()helper, applied to the four exposed specs.The spec now has to be able to fail
Two defects in my own test, both caught before shipping:
It compares
menu/pagesonly (the GET returns an effective manifest withruntimeinjected andnameadded), and gets its own fixture (pw-rollback) since it mutates the active manifest twice per run whileversionRoutingreads/manifest?_version=offpw-verchain.Three-way verified on the current spec: pre-fix fails (planted manifest survives the rollback), fixed passes 2/2.
Not addressed here
versionRoutinghas two failures on the shared instance, both independent of this change (they persist with separate fixtures):non-member must receive 404→ 401, becauseglobalSetupcannot mint therbac-outsidersession.editor must receive the staging manifest→ 404, unexplained; I have not investigated it.🤖 Generated with Claude Code