fix(procest): repair OR-API drift + dangling seed references by rubenvdlinde · Pull Request #417 · ConductionNL/procest

rubenvdlinde · 2026-05-12T04:56:19Z

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`).

github-actions · 2026-05-12T05:00:54Z

Quality Report — ConductionNL/procest @ `bf3fcea`

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

Spec coverage: 5% (21 tests / 456 specs)

Quality workflow — 2026-05-12 05:00 UTC

Download the full PDF report from the workflow artifacts.

…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.

github-actions · 2026-05-12T05:16:56Z

Quality Report — ConductionNL/procest @ `b6942cc`

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

Quality workflow — 2026-05-12 05:16 UTC

Download the full PDF report from the workflow artifacts.

github-actions · 2026-05-12T05:30:46Z

Quality Report — ConductionNL/procest @ `aebea2a`

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

Spec coverage: 5% (21 tests / 456 specs)

Quality workflow — 2026-05-12 05:30 UTC

Download the full PDF report from the workflow artifacts.

Continues the procest quality cleanup (after the phpcs commits): **stylelint** — added the missing peer deps of `@nextcloud/stylelint-config` (which the config `extends`): `stylelint-config-recommended-scss@^13.1.0`, `stylelint-config-recommended-vue@^1.6.1`, `postcss-html@^1.8.1` (matches decidesk's set). `npm run stylelint` now runs and reports **0 errors** — the "Vue Quality (stylelint)" gate was failing purely on the missing-config-module error, not on actual style violations. **eslint** — `npm run lint -- --fix` auto-fixed 5; the remaining 9 errors fixed by hand: - `bezwaar.js` — removed unused `settingsStore` var + its now-unused import - `MyWork.vue` — `<template v-else-if="!loading">` → `v-if` (the v-else-if duplicated the preceding `<ParafeerInbox v-if="!loading">` condition → the grouped sections could never render; `vue/no-dupe-v-else-if`) - `DurationPicker.vue` — dropped unused `formatDuration` import - `CaseTransferDialog.vue` / `CreateShareDialog.vue` — moved `<template #actions>` out of the wrapping `<div>` to be a direct child of `<NcDialog>` (named slots must be owned by the component, not a nested div; `vue/valid-v-slot`) - `EmailComposer.vue` — `{{ '{{' + v + '}}' }}` → `{{ '{' + '{' + v + '}' + '}' }}` (a literal `}}` inside a `{{ }}` interpolation made Vue's template parser see the interpolation close early → "Unterminated string constant"; splitting the braces avoids it, renders identically) - `WorkflowCanvas.vue` — `v-model:nodes`/`v-model:edges` (Vue-3 named v-model) → `:nodes.sync`/`:edges.sync` (Vue-2 equivalent). This file is dormant anyway (see below). `npm run lint` now exits 0 (errors=0; 14 warnings remain — `@nextcloud/no-deprecations` on `OC.currentUser` etc. — tolerated). **build** — `customComponents.js` no longer imports `VisualWorkflowEditor` (→ `WorkflowCanvas` → `@vue-flow/{core,controls,background}` v1.x, which are Vue-3-only and were breaking `npm run build` with 272 errors). The workflow component files stay in `src/components/workflow/` but dormant; re-wire when @vue-flow is replaced with a Vue-2-compatible flow lib. (Same fix as #417.) `npm run build` now compiles with 0 errors; `procest-main.js` parses clean. Together with the phpcs commits on this branch, procest's phpcs / eslint / stylelint / build CI checks are all green. Still red: Playwright E2E and the PHPUnit/Newman jobs (the latter fail on the OR-side `oc_openregister_objects` fresh-install issue — openregister#1492 — not a procest problem).

WilcoLouwerse · 2026-05-12T15:36:45Z

+							{
+								"type": "data"
+							}
+						],


[BLOCKER] Admin-only pages now accessible without page-level permission guard

Four pages that previously had "permission": "admin" at the page level have had that guard dropped during the reformatting pass:

/settings/wms-layers (WmsLayers index)

/settings/wms-layers/:id (Kaartlaag detail)

/settings/lhs-matrices (Handhavingsstrategie index)

/settings/lhs-matrices/:id (LHS Matrix detail)

The corresponding menu items still carry "permission": "admin" so non-admins won't see the links in the sidebar — but the manifest page objects themselves no longer enforce access. Any user who knows or guesses the URL can navigate directly to these admin-only pages, bypassing the menu-level guard. This is a privilege-escalation path that opens WMS layer management (external tile server credentials) and LHS enforcement-strategy matrices to all authenticated users.

Fix: restore "permission": "admin" as a top-level sibling of "route" on each of the four affected page objects, exactly as it appeared before this PR.

WilcoLouwerse · 2026-05-12T15:36:48Z

[BLOCKER] hasRow() called on flat findAll() array — shape mismatch may cause unchecked re-seeding

$existing is now the flat array returned by findAll() (a list of entity objects). The old getObjects() returned a paginated wrapper ['objects' => [...], 'total' => N]. The call $this->hasRow($existing) on line 61 is unchanged in this PR.

If hasRow() internally checks $existing['objects'] or treats a non-empty paginated wrapper as truthy, it will now receive an unexpected shape and silently return false even when an active LHS matrix already exists — causing the repair step to overwrite live matrix data on every run of php occ maintenance:repair.

The diff does not show the body of hasRow(). Before merging, verify that hasRow() correctly handles a flat PHP array. If it does not, inline the check as count($existing) > 0 or update hasRow() accordingly.

WilcoLouwerse · 2026-05-12T15:36:50Z

[CONCERN] PHP + operator silently discards caller-supplied register/schema overrides

The filter array is constructed as ['register' => $registerId, 'schema' => $schemaId] + $filters. PHP's + operator keeps left-hand keys on collision, so any register or schema key in $filters is silently discarded. The same pattern appears in LhsRecommendationService.php line 175.

This is likely intentional (callers should not override register/schema), but it creates a silent footgun: a caller passing those keys will see no error and get unexpected query results. At minimum, add a brief comment explaining the merge order is deliberate.

WilcoLouwerse · 2026-05-12T15:36:51Z

[CONCERN] Incomplete OR-API migration: TenantService::getTenantByGroupId() still uses getObjects()

The PR description acknowledges that TenantService::getTenantByGroupId() still calls the removed getObjects() API and claims it is safe because execution short-circuits before reaching it. This is a fragile assumption: any refactor of the short-circuit condition or the caller chain could cause a fatal runtime error in production.

Since this PR is scoped to repair OR-API drift, leaving a known broken call site in-tree is an incomplete fix. Add a // TODO(#411): migrate getObjects() — safe only while <condition> holds comment directly on the call site so the next contributor can see the risk, and make sure PR #411 is linked as a blocking follow-up.

WilcoLouwerse · 2026-05-12T15:36:53Z

[CONCERN] Seed deletion not verified for reverse references to deleted parafeeractie slugs

Three parafeeractie seed objects are removed because they contain dangling voorstel references. However, the PR does not confirm that no other remaining seed objects reference the deleted slugs themselves (parafeeractie-stap1-advies-2026-0042, parafeeractie-stap2-parafering-2026-0042, parafeeractie-stap2-geretourneerd-2026-0055).

If any surviving seed object references these slugs (e.g. a workflow step, a status record, or a voorstel body field), the deletion creates new dangling references in the opposite direction. Run grep -r 'parafeeractie-stap1-advies-2026-0042\|parafeeractie-stap2-parafering-2026-0042\|parafeeractie-stap2-geretourneerd-2026-0055' lib/Settings/ before merging.

WilcoLouwerse · 2026-05-12T15:36:54Z

+					},
+					{
+						"id": "cases-by-type",
+						"type": "custom",


[CONCERN] Manifest version not bumped despite schema-breaking structural changes

The manifest is described as 'conformed to canonical app-manifest schema v1.4.0', yet "version": "1.0.0" is unchanged. Actions arrays were changed from shorthand strings to full objects — a structural change. If the manifest version field is consumed by the loader or CI, leaving it at 1.0.0 is misleading. Consider bumping it to match the target schema version or the app's own semantic version for this release.

WilcoLouwerse · 2026-05-12T15:36:55Z

+					},
+					{
+						"id": "cases-by-type",
+						"type": "custom",


[CONCERN] 25 additionalProperties schema validation errors left unresolved

The PR description explicitly acknowledges 25 additionalProperties validation errors remain after the schema migration. The manifest declares a $schema URL but does not fully validate against it. Any schema-validation CI gate will continue to fail. These errors should be enumerated in a follow-up issue with an acceptance deadline, not left as open-ended known debt in the PR description.

WilcoLouwerse · 2026-05-12T15:36:56Z

+					},
+					{
+						"id": "cases-by-type",
+						"type": "custom",


[CONCERN] PR has merge conflicts with base branch development — must rebase before merge

The PR is currently marked CONFLICTING with development. The permission-guard omissions for WmsLayers and LhsMatrices pages may have been introduced accidentally during conflict resolution rather than being deliberate decisions. The author must rebase (not merge) against the current tip of development, resolve all conflicts, and re-request review. Approving a conflicting PR risks missing additional regressions introduced during the conflict resolution.

WilcoLouwerse · 2026-05-12T15:36:58Z

[CONCERN] VisualWorkflowEditor commented out without a tracking issue

VisualWorkflowEditor is unwired because @vue-flow/core v1.x requires Vue 3 and procest runs on Vue 2.7. The comment is informative but there is no linked GitHub issue to track resolution. Without a tracked issue, this can silently rot and the src/components/workflow/ files will confuse contributors.

Fix: open a tracking issue (e.g. 'Replace @vue-flow with Vue-2-compatible library or migrate to Vue 3'), reference the issue number in the comment, and optionally add a // @deprecated tag to the workflow component files.

WilcoLouwerse

Review

🔴 Blockers (2)

Admin-only pages now accessible without page-level permission guard — src/manifest.json:1341
hasRow() called on flat findAll() array — shape mismatch may cause unchecked re-seeding — lib/Repair/SeedLhsMatrix.php:61

🟡 Concerns (7)

PHP + operator silently discards caller-supplied register/schema overrides — lib/Service/SeedDataService.php:154
Incomplete OR-API migration: TenantService::getTenantByGroupId() still uses getObjects() — lib/Service/AppointmentService.php:75
Seed deletion not verified for reverse references to deleted parafeeractie slugs — lib/Settings/procest_register.json:189
Manifest version not bumped despite schema-breaking structural changes — src/manifest.json:273
25 additionalProperties schema validation errors left unresolved — src/manifest.json:273
PR has merge conflicts with base branch development — must rebase before merge — src/manifest.json:273
VisualWorkflowEditor commented out without a tracking issue — src/customComponents.js:251

🟢 Minor (1)

Inconsistent (int) casting of register/schema across findAll() call sites (lib/BackgroundJob/AppointmentReminderJob.php:14)
BackgroundJob files cast register and schema config values to (int) before passing them to findAll(). SeedLhsMatrix.php and LhsRecommendationService.php pass them as-is. If OR's findAll() API performs strict type comparison on filter values, mixed string/int inputs could produce silent empty results. Standardise the cast pattern across all call sites.

Reviewed by WilcoLouwerse via automated batch review.

rubenvdlinde requested review from WilcoLouwerse, bbrands02 and rjzondervan as code owners May 12, 2026 04:56

rubenvdlinde requested review from SudoThijn and remko48 as code owners May 12, 2026 05:09

rubenvdlinde mentioned this pull request May 12, 2026

chore(phpcs): clear procest's PHPCS baseline (phpcbf + manual) #421

Merged

WilcoLouwerse reviewed May 12, 2026

View reviewed changes

WilcoLouwerse requested changes May 12, 2026

View reviewed changes

Conversation

rubenvdlinde commented May 12, 2026

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

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

Out of scope (follow-ups)

Test plan

Uh oh!

github-actions Bot commented May 12, 2026

Quality Report — ConductionNL/procest @ bf3fcea

Uh oh!

github-actions Bot commented May 12, 2026

Quality Report — ConductionNL/procest @ b6942cc

Uh oh!

github-actions Bot commented May 12, 2026

Quality Report — ConductionNL/procest @ aebea2a

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilcoLouwerse left a comment

Choose a reason for hiding this comment

Review

🔴 Blockers (2)

🟡 Concerns (7)

🟢 Minor (1)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

Quality Report — ConductionNL/procest @ `bf3fcea`

Quality Report — ConductionNL/procest @ `b6942cc`

Quality Report — ConductionNL/procest @ `aebea2a`