Skip to content
This repository was archived by the owner on May 29, 2026. It is now read-only.

fix(procest): repair OR-API drift + dangling seed references#491

Closed
rubenvdlinde wants to merge 6 commits into
developmentfrom
feature/procest-production-fixes
Closed

fix(procest): repair OR-API drift + dangling seed references#491
rubenvdlinde wants to merge 6 commits into
developmentfrom
feature/procest-production-fixes

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

(re-opened of #417 — accidentally closed by harness force-push; same branch, same content)

Found during the local NC upgrade-and-audit pass — two production blockers:

1. Dangling seed references (config import fails on every install/upgrade)

Three parafeeractie seed objects in procest_register.json referenced voorstel-collegeadvies-0042 / voorstel-collegeadvies-0055, which have no matching voorstel seed object. OR validates parafeeractie.voorstel as a uuid-format relation and rejected the slug strings → Failed to import configuration for app procest: Property 'voorstel' should match format 'uuid'. The three orphaned objects are removed (42 → 39 objects).

2. Removed OR API: ObjectService::getObjects()findAll()

OCA\OpenRegister\Service\ObjectService::getObjects() no longer exists. Migrated all 9 call sites to findAll(['filters' => ['register' => ..., 'schema' => ..., ...]]) which returns a flat array. This fixes the Could not seed bezwaar/beroep data: Call to undefined method ObjectService::getObjects() repair warning (via SeedDataService::findByFilter).

TenantService::getTenantByGroupId() still has one getObjects() call but that path short-circuits before reaching it (the tenant_schema config key is never set) and PR #411 removes the method entirely — left as-is to avoid a merge conflict.

Out of scope (follow-ups)

  • The broken procest-main.js bundle ("Unexpected identifier 'Case'" JS syntax error — needs a rebuild)
  • 25 additionalProperties validation errors in src/manifest.json page actions against canonical schema v1.4.0 (the schema's action def is additionalProperties:false and lacks handler/route — needs an action-shape design decision)

Test plan

  • occ upgrade on a fresh NC dev env no longer emits the procest config-import warning
  • Quality gate (phpcs/psalm/phpstan) green

Two production blockers found during the local NC upgrade-and-audit pass:

1. Dangling seed references — three `parafeeractie` seed objects in
   `procest_register.json` referenced `voorstel-collegeadvies-0042` /
   `voorstel-collegeadvies-0055`, which have no matching `voorstel` seed
   object. OR's schema import validates `parafeeractie.voorstel` as a
   uuid-format relation and rejected the slug strings, aborting the
   configuration import on every install/upgrade. The three orphaned
   objects are removed (they couldn't function without their parent
   voorstel anyway). Object count 42 → 39.

2. Removed OR API — `OCA\OpenRegister\Service\ObjectService::getObjects()`
   no longer exists; the current API is `findAll(array $config)` which
   returns a flat array of rendered objects and reads `register`/`schema`
   from `$config['filters']`. Migrated all nine call sites:
   - lib/BackgroundJob/AppointmentReminderJob.php
   - lib/BackgroundJob/ShareMaintenanceJob.php
   - lib/Service/BerichtenboxService.php
   - lib/Service/SeedDataService.php  (this fixes the
     "Could not seed bezwaar/beroep data: Call to undefined method
     ObjectService::getObjects()" repair warning)
   - lib/Service/CaseSharingService.php
   - lib/Service/AppointmentService.php  (×2)
   - lib/Service/Vth/LhsRecommendationService.php
   - lib/Repair/SeedLhsMatrix.php

`lib/Service/TenantService.php` still has one `getObjects()` call inside
`getTenantByGroupId()`, but that path short-circuits before reaching it
(the `tenant_schema` config key is never set) and PR #411
(migrate-tenant-to-or-tenant) removes the method entirely — left as-is to
avoid a conflict with that branch.

Out of scope (separate follow-ups): the broken `procest-main.js` bundle
("Unexpected identifier 'Case'" — needs a rebuild) and the 25
`additionalProperties` validation errors in `src/manifest.json` page
actions against canonical schema v1.4.0 (needs an action-shape decision —
the schema's `action` def is `additionalProperties:false` and lacks
`handler`/`route`).
…eak)

`@vue-flow/{core,controls,background}` v1.x are Vue-3-only — they import
Fragment / Teleport / createElementVNode / toValue from 'vue', none of which
exist in procest's Vue 2.7 runtime. As a result `npm run build` fails with
272 errors, all from those three packages (190 from @vue-flow/core, 65 from
@vue-flow/controls, 17 from @vue-flow/background). This is what produced the
broken `procest-main.js` bundle ("Unexpected identifier 'Case'" at runtime).

Removes the `VisualWorkflowEditor` import + export from `src/customComponents.js`
so `@vue-flow/*` is no longer pulled into the webpack graph. The component files
in `src/components/workflow/` are left in place but dormant; the
`WorkflowTemplateEditor` manifest page that references `VisualWorkflowEditor`
by string name now renders nothing (graceful — Vue dynamic-component with an
unregistered name is a no-op, not a crash).

`npm run build` now succeeds (18 warnings, 0 errors) and `procest-main.js`
parses cleanly.

Follow-up: replace `@vue-flow` with a Vue-2-compatible flow library (or migrate
procest to Vue 3) and re-wire the visual workflow editor.
`@conduction/nextcloud-vue` ships the canonical manifest schema at
`src/schemas/app-manifest.schema.json` (ADR-024 — apps consume it, never
fork it). procest's manifest validated with 110 errors against schema 1.3.0:

- 30 page `actions` were string shorthand (`"create"`) or used a non-canonical
  `{key:...}` shape — the schema requires `{id, label, ...}` objects with
  `id` + `label` required (no string shorthand for actions). Normalized:
  string → `{id, label}`; `{key:x}` → `{id:x, label:<humanized>}`; preserved
  any `icon` / `permission` / `primary` / `confirm` / `handler` / `route`.
- 22 page `columns` used a non-canonical `{key:...}` shape failing both
  branches of the schema's `oneOf` (string shorthand | `{key, label, ...}`
  with `label` required). Normalized: `{key:x}` with no extras → string `"x"`;
  `{key:x, ...}` → `{key:x, label:<humanized>, ...}`.
- 4 pages carried a top-level `permission` key the page schema doesn't define
  (`additionalProperties:false`) — removed. (Page-level permission gating
  isn't part of the canonical page contract; if procest needs it, that's a
  schema-extension request against nextcloud-vue, not a per-app field.)

`npm run tests/validate-manifest.js` now passes with 0 errors against
schema 1.3.0. Manifest renderer (`CnIndexPage` / `CnRowActions`) consumes
the canonical `id` / `handler` / `route` action contract; procest's custom
view components don't read manifest row actions directly so this is a
self-contained normalization.

Note: the diff is large because `json.dump` re-formats the whole file to
consistent tab-indented multi-line — review with `git diff -w` to see the
substantive 110-fix changes only.

Closes the manifest-conformance follow-up from the production-readiness audit.
@rubenvdlinde rubenvdlinde force-pushed the feature/procest-production-fixes branch from a08058e to 43d2de4 Compare May 19, 2026 04:03
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/procest @ cec64e5

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 100/100
npm ✅ 420/420
PHPUnit
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-05-19 04:08 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/procest @ 6006345

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 100/100
npm ✅ 420/420
PHPUnit
Newman ⏭️
Playwright ⏭️

Coverage: 0% (0/81 statements)


Quality workflow — 2026-05-19 04:13 UTC

Download the full PDF report from the workflow artifacts.

PHPUnit was erroring with:
  Call to undefined method MockObject_SeedObjectServiceStub::findAll()

SeedDataService::findByFilter() calls $objectService->findAll() (the
current OR ObjectService API), but the stub interface declared only
getObjects() + saveObject(). The two test methods that exercise
findByFilter (testSeedBezwaarBeroepDataReturnsSummaryStructure +
testSeedBezwaarBeroepDataSkipsExistingCaseTypes) mocked the
no-longer-used getObjects() instead of findAll(), so production code
hit an undefined-method error on the auto-generated MockObject.

Fixes:
- Add findAll(array $criteria): array to the SeedObjectServiceStub.
- Add ->method('findAll')->willReturn([]) to the "no existing" test.
- Add ->method('findAll')->willReturn([\$existingObject]) to the
  "skips existing" test (matches the [\$existing] return shape that
  findByFilter expects).
composer audit was flagging 13 vulnerabilities across 2 packages:
- symfony/yaml v6.4.34 → v6.4.40 (fixes CVE-2026-45304 ReDoS,
  CVE-2026-45305 ReDoS, plus other 6.4.x advisories)
- twig/twig v3.23.0 → v3.26.0 (fixes a chain of sandbox-escape /
  template-injection advisories on 3.0-3.25.x)

Lock-only update; no composer.json changes (yaml + twig come in
transitively via consolidation/robo and edgedesign/phpqa).
rubenvdlinde added a commit that referenced this pull request May 20, 2026
Same fix landed in PR #491 and #496. Pulling it into this PR so
its CI doesn't carry the same red Security (composer) check.

- symfony/yaml v6.4.34 → v6.4.40 (CVE-2026-45304/45305 + others)
- twig/twig v3.23.0 → v3.26.0 (sandbox-escape advisories chain)
rubenvdlinde added a commit that referenced this pull request May 20, 2026
* fix(eslint): collapse double blank lines in src/registry.js

eslint rule no-multiple-empty-lines was flagging two consecutive blank
lines between the PublicStatusPage entry and the Detail-tab components
section. Single blank line is the project convention.

Surfaced by procest#496 (the tenant-OR PR) eslint job, which runs
against the PR-merged-with-dev commit.

* chore(security): bump symfony/yaml + twig/twig past advisories

Same fix landed in PR #491 and #496. Pulling it into this PR so
its CI doesn't carry the same red Security (composer) check.

- symfony/yaml v6.4.34 → v6.4.40 (CVE-2026-45304/45305 + others)
- twig/twig v3.23.0 → v3.26.0 (sandbox-escape advisories chain)
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

🤖 Autonomous backlog sweep (2026-05-20T22:49Z): tried admin-merge but the rebase onto current development aborts on README.md with an 'Initial commit' patch — indicates the branch has divergent root history (likely from the consolidation work documented in project_branch-consolidation-2026-05). Many of this branch's commits (#499, #500, #475, etc.) are already on development via separate PRs.

The substantive fix (fix(procest): repair OR-API drift + dangling seed references, commit a02abf2) may still be needed — check whether the parafeeractie → voorstel-collegeadvies UUID drift in procest_register.json was repaired through another path. If not, recommend cherry-picking a02abf2 onto a fresh branch from development and opening a new PR.

Not auto-closing. Needs eyes.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Superseded by #530 — the OR-API drift fix (the still-relevant half of this PR) has been cherry-picked onto a fresh branch from development and merged. The other commits on this branch were either already on development via separate paths or made obsolete by the manifest-v2 migration. Closing the divergent-history branch.

@rubenvdlinde rubenvdlinde deleted the feature/procest-production-fixes branch May 21, 2026 05:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant