Repository navigation
feat(cmdb-import): run the mapping packs stored in OpenRegister, seeded from the shipped files - #1288
WilcoLouwerse wants to merge 12 commits into
Conversation
Adds the OpenSpec change cmdb-import-mapping-view (ADDED REQ-CMDB-020,
MODIFIED REQ-CMDB-014) and the admin-only GET
/api/settings/cmdb-import/mapping. It loads the import profile and its five
packs through CmdbImportProfile, the loader the import uses, and answers the
flat {profile, packs}; a broken pack answers 503 MAPPING_UNAVAILABLE with
the loader's reason.
First of two slices of #1280.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Quality Report — ConductionNL/stackiq @
|
| 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 | ✅ | ✅ 133/133 | |||
| npm | ✅ | ✅ 807/807 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ✅ 1230/1230 · 20 skipped | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-10-12 10:38 UTC
Download the full PDF report from the workflow artifacts.
…line The mapping endpoint returned the loader's exception message as details.reason unbounded, and its docblock and design D2 said that message named files and validator rules only. The validator's messages also quote the configuration value they refused, and nothing limited their length. The reason is now the message's first line, cut to 500 characters, and both texts say what it carries: the file, the rule and the refused value, never a workbook cell or a person. Mutation-checked: testTheReasonIsOneBoundedLine fails when the endpoint returns the raw message and passes with the bound. phpcs, phpmd and phpstan report 0; the CMDB and SettingsController suites pass (306 tests). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The cmdb-import-mapping-view delta now says details.reason is one line of at most 500 characters and may quote the refused configuration value, as the endpoint and design D2 do since the reason is bounded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| array_column($data['packs'], 'file') | ||
| ); | ||
|
|
||
| // What is shown is what runs: every pack lists as many mappings as the loader hands the engine. |
There was a problem hiding this comment.
🟢 Minor — 'Same loader' and 403 are asserted by no test
Contract-branch table: (1) 'the same loader the import uses': the test builds new CmdbImportProfile(...) itself, passes it in, and compares the answer with $profile->pack() of the same object, so it would stay green if the controller's fallback built a different directory or class; nothing ties it to the instance CmdbExportImportService gets (both are container-autowired today: no CmdbImportProfile registration in Application.php, so the claim holds, unpinned). (2) 'non-admin 403': only static reflection (no attribute) is tested; the 403 itself is the rig check in the PR body and the e2e in #1289. (3) 503 branches 'profile unreadable' and 'profile names no pack for target' have no test at either level (only a missing pack file and an invalid pack are covered).
Suggested fix: one test that resolves the controller's default (cmdbImportProfile: null) overview and the overview of new CmdbImportProfile(container: ...) and asserts equal; one profile-level test for a profile without packs.usage.
Done when:
- the null-profile controller test goes red when the fallback in getCmdbImportMapping() is pointed at another directory
| [ | ||
| 'success' => false, | ||
| 'error' => $e->getErrorCode(), | ||
| 'message' => $this->translate(text: 'The import mapping cannot be shown: OpenRegister is missing or a mapping file is invalid.'), |
There was a problem hiding this comment.
🟢 Minor — New 503/500 messages are not extractable and not translated in this slice
The two user-facing messages go through a private translate(string $text) that calls $this->l10n->t($text) with a variable. Nextcloud's translation tooling extracts literal ->t('...') calls, and tests/l10n/check-l10n.js does not scan PHP at all, so nothing ensures these strings reach l10n/nl.json. The l10n files are in #1289; with this slice alone git grep -n -e 'cannot be shown' -- l10n is empty, so a Dutch admin gets English until the second PR lands (REQ-CMDB-020/ADR-005 require Dutch and English for the error).
Suggested fix: call $this->l10n->t('...') with the literal string (CmdbImportController does), and keep the null-translator fallback in the constructor default instead of a wrapper; or note in the PR body that the slice ships English-only until #1289.
| - GIVEN the endpoint's answer WHEN the block is expanded THEN one table per pack renders with `data-testid="cmdb-import-mapping-<target>"`, the usage table holding "Applicatie Status" → `status` with the lookup pairs | ||
| - GIVEN a 503 WHEN the block loads THEN it shows the error and the code and no table | ||
| - GIVEN the answer WHEN the section renders its help text THEN the sheet names are the endpoint's; before and on failure they are `PROFILE_DEFAULTS.sheets` | ||
| - [x] Implement |
There was a problem hiding this comment.
🟢 Minor — tasks.md marks Tasks 3-5 implemented; their files are not in this slice
Task 3 (CmdbImportMapping.vue, vitest), Task 4 (l10n, docs section) and Task 5 (e2e) carry [x] Implement, and the spec's @e2e tests/e2e/spec-coverage/cmdb-import-mapping.spec.ts points at a file that does not exist at this head (checked: CmdbImportMapping.vue, cmdbImportMapping.spec.js, cmdb-import-mapping.spec.ts are all absent). They arrive with #1289, so this is correct for the stack but wrong for #1288 alone, and if #1289 is split or reordered the checkboxes mislead. Not blocking; flag so the stack order is deliberate.
Suggested fix: none required; tick Tasks 3-5 in #1289.
Split artefact: this slice was cut from a larger PR; the item resolves once the stack above it merges. Fix only if this slice may merge on its own for a while.
Quality Report — ConductionNL/stackiq @
|
| 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 | ✅ | ✅ 133/133 | |||
| npm | ✅ | ✅ 807/807 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ✅ 1231/1231 · 20 skipped | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-10-12 12:52 UTC
Download the full PDF report from the workflow artifacts.
There was a problem hiding this comment.
Verdict: APPROVE (Thorough) — self-review posted as COMMENT (GitHub blocks self-APPROVE)
No blockers: the endpoint is admin-only with CSRF, answers flat, logs unexpected errors with a static 500, and loads the profile through the same autowired CmdbImportProfile as the import. The unbounded failure reason was fixed before posting; what remains is that the view does not yet show the import's write rules.
Update 2026-10-12: The remaining concern was fixed on the branch after posting.
🟡 Concerns (1)
- Overview omits the write rules, so 'what the next import runs' is overclaimed —
lib/Service/Cmdb/CmdbImportProfile.php:629. The docblock (and SettingsController.php:3899, design.md:11, proposal.md:10, spec.md:11) promise that the answer is exactly what the next import runs. ✅ Addressed inecc9c8b0, df28777e, 6ecb274a.
🟢 Minor (3)
- 'Same loader' and 403 are asserted by no test —
tests/Unit/Controller/SettingsControllerCmdbImportMappingTest.php:225. Contract-branch table: (1) 'the same loader the import uses': the test buildsnew CmdbImportProfile(...)itself, passes it in, and compares the answer with$profile->pack()of the same object, so it would stay green if the controller's fallback built a different directory or class; nothing ties it to the instance CmdbExportImportService gets (both are container-autowired today: no CmdbImportProfile registration … - New 503/500 messages are not extractable and not translated in this slice —
lib/Controller/SettingsController.php:3935. The two user-facing messages go through a privatetranslate(string $text)that calls$this->l10n->t($text)with a variable. - tasks.md marks Tasks 3-5 implemented; their files are not in this slice —
openspec/changes/cmdb-import-mapping-view/tasks.md:34. Task 3 (CmdbImportMapping.vue, vitest), Task 4 (l10n, docs section) and Task 5 (e2e) carry[x] Implement, and the spec's@e2e tests/e2e/spec-coverage/cmdb-import-mapping.spec.tspoints at a file that does not exist at this head (checked: CmdbImportMapping.vue, cmdbImportMapping.spec.js, cmdb-import-mapping.spec.ts are all absent).
✅ Fixed before posting (1)
- details.reason returns $e->getMessage() to the client — fixed in
0108ef0.
✅ Verified clean
- No auth attribute (Nextcloud default: admin + CSRF) with
@auth admin-only <reason>, asserted bytestTheRouteIsForNextcloudAdminsOnly. - 503 path: no absolute path or stack trace reaches the body; the 500 path returns a static message and logs the exception.
requiredis always a boolean; the spec'sdone→in-progressflip follows writing-specs' re-opening rule.
Scope decision: slice 1 of 2 of the closed #1280, stacked on development; reviewed against its own base only.
Gates: CI quality / Hydra Gates and the required PHP/Vue quality checks green on {h}; quality / PHPUnit was still running at submission.
ADRs verified: ADR-005, ADR-012, ADR-031, ADR-050 — all resolve on hydra main.
Jira context: WOO-588 — CMDB-import: mapping inzichtelijk en beheerbaar voor de beheerder (status: Refinement)
…view The overview listed the pack field mappings only, so the usage fields interneAnnotation and timeClassification looked like ordinary mappings while the import writes them on create or when the stored value is empty, and the module type Application and the never-written publication dates showed nowhere. mappingOverview() now answers per pack `defaults`, `createOnlyDefaults`, `createOnlyFields` and `neverWritten`, built from the accessors the import uses, plus the profile's `externalKeyPrefix`. REQ-CMDB-020, tasks.md and design.md name the new keys; the controller and profile tests pin them. Review #1288 f2 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
createOnlyFields is documented as mapped fields, but for the module pack the accessor returned ['type'], a key of the create-only defaults that merge() never reads on update (it only walks mapped fields). The overview now answers that value under createOnlyDefaults only and subtracts the default keys from createOnlyFields; the import's accessors are unchanged. REQ-CMDB-020 and design.md say so, and the controller and profile tests pin it. Review pre-push p6 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…OnlyFields The overview subtracted every key of createOnlyDefaults from createOnlyFields. merge() guards a mapped create-only field on update, so a mapped field that also had a create default would have shown as written on every update. Only defaults that are not mapped fields leave the list now (the shipped module type); REQ-CMDB-020, design and the overlap assertions say the same. Mutation-checked: testAMappedCreateOnlyDefaultStaysACreateOnlyField fails on the previous subtraction and passes now. Review pre-push q1 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The mapping tables listed every mapped field as written on each import, but the import writes the usage's interneAnnotation and timeClassification only on create or when the stored value is empty, sets the module type on create, and never writes the module's publication dates on an existing record. The block now shows, per row, when the import writes the field, and under each table the pack defaults, the create-only values and the never-written fields that the endpoint answers since the previous slice. New strings in en and nl; REQ-CMDB-020, the user docs, the vitest and the e2e API check name them. Review #1288 f2 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Quality Report — ConductionNL/stackiq @
|
| 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 | ✅ | ✅ 133/133 | |||
| npm | ✅ | ✅ 807/807 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ✅ 1232/1232 · 20 skipped | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-10-12 16:17 UTC
Download the full PDF report from the workflow artifacts.
The module, manufacturer, usage and business-owner packs now live in OpenRegister's migration-pack store, where an administrator edits them, and the import runs what is stored there. A repair step seeds missing packs as built-in packs and replaces a pack nobody edited when a release ships a newer one; an x-stackiq marker (shipped version and the hash of the shipped pack) tells an edit from a release, and an edited pack is never overwritten. The shipped files stay the seed, and the fallback while nothing was stored. Every pack must follow the import's rules: the required name and APPID mappings, and only fields of its target schema, never object metadata, match keys or the relations the import sets. An invalid or rule-breaking pack, a renamed pack, or a store that is gone after a seed answers 503 MAPPING_UNAVAILABLE naming the pack and its id. A column a pack maps with a date transform is converted from an Excel date, and the note cleanup uses the pack's own separator. The municipality pack stays a shipped file. GET /api/settings/cmdb-import/mapping answers per pack its source, OpenRegister id, stored and shipped versions, edited and newerShipped, editable and an editUrl to OpenRegister's pack editor, plus openRegisterPacks. The OpenSpec change and the documentation describe editing in OpenRegister. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…rdened pack stores The import's rules took every property of the target schema as writable, so an edited pack could map relations (organization.participants writes back into other organisations), publication dates and admin-only fields, all saved with RBAC and multitenancy off. Each target now has an explicit allowlist of vetted plain fields: what the shipped packs map, the module's licentietype and the usage's phase start dates. Relations ($ref, inversedBy, writeBack, uuid, related objects, on the property or its items), publication dates and admin fields are refused as mappings and as defaults; the organisation status and registeredBy defaults stay pinned. OpenRegister's pack store now counts as available only when BuiltinPackSlugLockedException exists, i.e. on the OpenRegister whose pack writes require CSRF and lock a built-in slug. Released versions keep the shipped files with a warning, and fail closed once ids were recorded. A seed that loses a create race to another request uses the row that request stored instead of answering 503. The note cleanup keeps the concat separator when a later plain mapping of the same field exists, because the engine skips a mapping whose column is empty. The mapping endpoint's 503 for a stored pack carries details.editUrl to its editor. The spec delta keeps only scenarios an e2e test proves: the new cmdb-import-packs.spec.ts drives the store, the mapping and the import through the API. Store-removed, install and upgrade behaviour stays in the requirement text and PHPUnit; the UI scenarios move to the UI change. Review findings S1, S2, F2 (endpoint part), F5, F6, F7 (endpoint part) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…es it The loader's docblock still said a pack may map every field of its target schema; it now names the rule CmdbImportPackRules applies. Review finding S1 (sweep) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…pgrade SeedCmdbImportPacks is registered as a post-migration repair step, but Nextcloud only runs repair steps when <version> in appinfo/info.xml moves. With the version unchanged, `occ upgrade` answers "No upgrade required." on an existing install and the packs are never seeded. 0.2.5-unstable.20261008120000 -> 0.2.6-unstable.20261012120000, in line with the 0.2.6 unstable releases already cut from development. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Quality Report — ConductionNL/stackiq @
|
| 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 | ✅ | ✅ 133/133 | |||
| npm | ✅ | ✅ 807/807 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ❌ 1307/1308 · 20 skipped | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-10-12 19:39 UTC
Download the full PDF report from the workflow artifacts.
Nextcloud's lib/base.php installs an external entity loader that returns null, so simplexml_load_file() returns false for a valid file when the tests run inside a Nextcloud checkout, as in CI. The test now loads the string, like ConnectionsDeclarationTest, and asserts the parse. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Quality Report — ConductionNL/stackiq @
|
| 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 | ✅ | ✅ 133/133 | |||
| npm | ✅ | ✅ 807/807 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ✅ 1308/1308 · 20 skipped | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-10-13 08:45 UTC
Download the full PDF report from the workflow artifacts.
What
The CMDB import runs the mapping packs stored in OpenRegister, where an admin views and edits them (ConductionNL/openregister#4602, the migration-pack editor). The packs under
lib/Settings/cmdb-import/stay the seed and the fallback. Rework of this PR's first version (read-only overview of the shipped files) after the decision of 2026-10-12: mappings are managed in OpenRegister.How
CmdbImportProfile,CmdbImportPackStore): module, manufacturer, usage and business owner come from OpenRegister's migration-pack store by slug; the municipality pack maps the import options and always comes from its file. A stored pack is validated byPackDefinitionValidatorand by the import's own rules (CmdbImportPackRules): required targets, and a per-pack allowlist of plain fields — relations, publication dates,@self, ids and admin-only fields are refused. A broken pack stops the import with 503MAPPING_UNAVAILABLEnaming the pack, never a silent fallback.SeedCmdbImportPacks, install + post-migration; version bumped so it runs on upgrade): the 4 packs are created as built-in with anx-stackiqmarker (shipped version + hash). A later release updates a stored pack that was not edited; an edited one is left alone and reported as "newer version shipped". A deleted pack is seeded again on the next import.GET /api/settings/cmdb-import/mappingadds per packsource,packId,storedVersion,shippedVersion,edited,newerShipped,editable,editUrl(IURLGenerator), and top-levelopenRegisterPacks.cmdb-import-mapping-viewrewritten for this scope (one mapping per instance);docs/features/cmdb-import.mdexplains viewing and editing in OpenRegister.Checks
PHPUnit (Cmdb services, the repair step, CmdbImportController, the mapping endpoint) 258 passed; hydra gates (local, diff scope) all applicable green incl. gate 110 (version bump).
Rig (NC 35; OpenRegister development + openregister#4602; fixtures only): the 4 packs are seeded (on an instance that upgraded stackiq before OpenRegister had the hardened store, the seed runs at the next repair or import); the endpoint reports
source: openregisterwith edit links; an edited pack is what the import runs (2/2 rows); a pack mappingproviderstops the import with 503 naming the pack; restoring the pack makes it "not edited" again.Merge order and conflicts
CmdbImportProfile.php,CmdbExportImportService.php, their tests,docs/features/cmdb-import.md, the main spec list and l10n: whichever merges second keeps both sides (the rig build merged them that way).Jira: WOO-588 — plan: https://github.com/ConductionNL/readonly-mirror-wilco-claude-plans/tree/dev-mirror/issues/jira/WOO-586/sub-tasks/WOO-588/
🤖 Generated with Claude Code