feat(cmdb-import): read the archive sheet and let module and usage be archived by WilcoLouwerse · Pull Request #1285 · ConductionNL/stackiq

WilcoLouwerse · 2026-10-12T10:24:25Z

What

First of three stacked PRs that replace #1281 (archive or soft-delete applications that left a newer CMDB export). This slice reads the archive sheet; nothing acts on it yet.

Spec: OpenSpec change cmdb-import-archive-reconciliation: ADDED REQ-CMDB-015..017, MODIFIED REQ-CMDB-012 and REQ-CMDB-014. The design records why object state is read per lens and from the trash (D9).
Schemas: module (0.3.9) and usage (1.5.7) opt in to archiving through x-openregister-archive. Run Force Update after installing.
Profile: the import profile names the sheet "Gearchiveerde Applicaties" and the accepted missingRecords modes (keep, archive), readable before the packs are validated.
Reader: when the workbook has the archive sheet, it is read under the same bounds as the CMDB sheets, for its APPID column only, and its APPIDs are returned next to the rows.
Fixture: topdesk-archived-applications.xlsx, a later export in which APPID 1234 moved to the archive sheet and APPID 2 is on no sheet.

Stack

This PR → development
feat(cmdb-import): archive or soft-delete applications that left the export #1286: the reconciliation in the import service and the endpoint option
feat(cmdb-import): offer the missing-records choice and an Archived quick filter #1287: the import section's choice, the Archived quick filter, e2e and docs

Jira: WOO-587 (sub-task of WOO-586). Plan: WOO-587 plan. Review and merge are tracked in WOO-590.

Review fixes (2026-10-12)

Commit	What
`388181a3`	The live spec names the renamed module-version test (039).
`16337d91`	An archive-sheet APPID without a cached value is reported (`uncachedRows`), not dropped; the spec delta, design D3/D5 and task 2 say what follows (archive, never delete — #1286).
`11d9a23c`	One test per bound the archive sheet is read under (part size, row span, referenced-string budget, key-column-only filter, `archiveSheetName()` null cases), each mutation-checked.
`aa3b6648`, `70d6f934`	Proposal, design and tasks name all three archive-only cases; design D8 describes the Archived tab ignoring facets and search.

Tests

Check	Result
PHPUnit CMDB, Settings and Fixtures suites on this slice alone	✅ 313 tests
phpcs / phpmd / phpstan on this slice alone	✅ 0 errors

🤖 Generated with Claude Code

… archived Adds the OpenSpec change cmdb-import-archive-reconciliation (REQ-CMDB-015..017, REQ-CMDB-012 and REQ-CMDB-014 modified). The register fragment opts module (0.3.9) and usage (1.5.7) into OpenRegister's archive state, the import profile names the sheet "Gearchiveerde Applicaties" and the accepted missingRecords modes, and the workbook reader returns the APPIDs of that sheet, read under the same bounds as the CMDB sheets for its key column only. The archived-applications fixture covers it. First of three slices of #1281; nothing reads the archive sheet yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-12T10:38:21Z

Quality Report — ConductionNL/stackiq @ `0c0afb5`

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					✅ 1233/1233 · 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.

…rt-import spec The fragment test was renamed to testTheMergedModuleIsVersion039WithTheExternalIds when the module schema moved to 0.3.9; the live spec still named the 038 test. The archived change keeps its historical name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

WilcoLouwerse · 2026-10-12T12:49:05Z

+  - GIVEN OpenRegister's search results (no archive marker; no soft-deleted row, `_includeDeleted` or not) THEN the state comes from the lens that found the object and from the trash listing, read once per import (design D9)
+  - GIVEN `updateExisting` false THEN the row is skipped as `exists` and nothing is revived
+  - GIVEN a deleted and a working module (and usage) sharing one import key THEN the working one is updated and nothing is restored
+- [x] Implement


🟢 Minor — tasks.md ticks tasks whose code is in #1286/#1287

🟡 Concern — tasks 3 to 7 are marked implemented and tested in a PR that contains none of that code

tasks.md ticks [x] Implement and [x] Test for Task 3 (matching revives, CmdbExportImportService.php, CmdbImportReport.php), Task 4 (reconciliation), Task 5 (controller option), Task 6 (settings section) and Task 7 (fixture, docs; only its Test is open). This diff touches none of those files, and the PR body says "nothing acts on it yet". Once this merges to development, anything that reads the checkboxes (/opsx-verify, /opsx-archive, a reviewer) sees a change that is nearly done.

Impact: the change's progress record is false on development between the merges of the three slices.

Suggested fix: leave Tasks 3 to 7 unticked here; tick each in the slice that implements it. Task 1 and Task 2 stay ticked.

Done when:

grep -c '^- \[x\]' openspec/changes/cmdb-import-archive-reconciliation/tasks.md returns 4 on this head

git diff origin/development --stat -- lib/Service/CmdbExportImportService.php lib/Controller is empty, matching the unticked tasks

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.

WilcoLouwerse · 2026-10-12T12:49:05Z

 			path: $path,
 			limit: $profile->maxReferencedStringBytes(),
-			sheetCount: count($profile->sheetNames()),
+			sheetCount: count($profile->readSheetNames()),


🟢 Minor — Referenced-string budget shrinks for workbooks without the archive sheet

🟢 Minor — count($profile->readSheetNames()) always counts the archive sheet, also when the workbook has none

assertReferencedStringBytes divides maxReferencedStringBytes by sheetCount (CmdbWorkbookBounds.php:207). The count goes from 2 to 3 for every workbook, so a workbook without the archive sheet now gets limit/3 (about 21 MiB of the 64 MiB default) instead of limit/2 (32 MiB). At the default limit this will not matter for a 10 MB upload, but it is a tightening nobody decided, the @param of assertReferencedStringBytes still says "the number of source sheets the profile reads", and the sheet list is only known after this call. Counting the sheets actually read would need sourceSheets() to run first. Optional: leave as is and say so in the docblock.

WilcoLouwerse · 2026-10-12T12:49:05Z

+					limit: $limit
+				);
+				$archive['present'] = true;
+				$archive['appIds'] = array_column(array_column($archiveRows, 'cells'), $profile->keyColumn());


🟢 Minor — appIds are raw cell values, not match keys

🟢 Minor — archive.appIds holds raw typed cell values; say so, or normalise

Read-only probe: cells 7.0, ' 8 ', 'app-9' come back as 7 (int), ' 8 ' and 'app-9'. The source rows go through the normaliser and matchKey() (trim, non-breaking space, lower case; winningSheets() does exactly that), so #1286 must apply the same to these or APP-1 on the archive sheet will not match app-1 on the CMDB sheet. The new tests hide the types behind array_map('strval', ...). The read() @return and design D3 do not say the values are raw (D3 also omits the sheet key the array carries).

Suggested fix: state in the @return that appIds are raw cell values to be compared through the normaliser and CmdbExportImportService::matchKey(), and assert the raw types in one test.

WilcoLouwerse · 2026-10-12T12:49:05Z

    "module": ["publicationDate", "depublicationDate"]
  },
-  "missingRecords": ["keep"]
+  "missingRecords": ["keep", "archive"]


🟢 Minor — Profile accepts 'archive' while the endpoint still refuses it

🟢 Minor — heads-up: until #1286 merges, the profile and the endpoint disagree

The profile now lists ["keep", "archive"] and missingRecordsModes() reads it, but nothing in lib/ calls missingRecordsModes() on this head (only the unit test does): CmdbExportImportService::supportsMissingRecords() is still $mode === 'keep' and CmdbImportController still answers details.accepted: ['keep']. This matches "nothing reads it yet" for the reader, but the profile is the declared source of the accepted values (spec REQ-CMDB-012, MODIFIED here), and for a release cut between the slices the declared value is one the endpoint refuses. Fine if the three slices always ship together; if not, say so in the PR stack note.

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.

WilcoLouwerse

Verdict: APPROVE (Thorough) — self-review posted as COMMENT (GitHub blocks self-APPROVE)

No blockers: the archive sheet is read under the CMDB sheets' bounds and the readBounded() refactor returns the same archive array in all three branches. The concerns are about silent gaps (uncached formulas, untested bounds) that matter once #1286 acts on these APPIDs.

Update 2026-10-12: All three concerns were fixed on the branch after posting; the four minors stay open as follow-ups.

🟡 Concerns (3)

Uncached archive-sheet formulas vanish without a warning — lib/Service/Cmdb/CmdbWorkbookReader.php:366. 🟡 Concern — the archive sheet drops a formula without a cached value silently, and reports present: true with fewer APPIDs The archive sheet is formula-derived: the new fixture's APPID cell is <f>..</f><v>1234</v> (build-fixtures.py, archive_appid). ✅ Addressed in 16337d91.
Archive sheet bounds now apply to imports that keep — lib/Service/Cmdb/CmdbWorkbookReader.php:187. 🟡 Concern — an export that imports today can be refused because of a sheet the admin chose not to act on read() takes no mode, so from this slice on every import, including missingRecords=keep, loads the archive sheet and enforces on it the part-size bound (here), the row span, the referenced-string budget and TOO_MANY_ROWS. ✅ Addressed in cdbec3d3 (#1287).
'Same bounds as the CMDB sheets' is tested for one of five bounds — tests/Unit/Service/Cmdb/CmdbWorkbookReaderTest.php:1005. 🟡 Concern — the totality promise "the archive sheet is read under the same bounds as the CMDB sheets, APPID column only" has one test REQ-CMDB-015, design D3 and the PR body promise that. ✅ Addressed in 11d9a23c.

🟢 Minor (4)

tasks.md ticks tasks whose code is in #1286/#1287 — openspec/changes/cmdb-import-archive-reconciliation/tasks.md:37. 🟡 Concern — tasks 3 to 7 are marked implemented and tested in a PR that contains none of that code tasks.md ticks [x] Implement and [x] Test for Task 3 (matching revives, CmdbExportImportService.php, CmdbImportReport.php), Task 4 (reconciliation), Task 5 (controller option), Task 6 (settings section) and Task 7 (fixture, docs; only its Test is open).
Referenced-string budget shrinks for workbooks without the archive sheet — lib/Service/Cmdb/CmdbWorkbookReader.php:313. 🟢 Minor — count($profile->readSheetNames()) always counts the archive sheet, also when the workbook has none assertReferencedStringBytes divides maxReferencedStringBytes by sheetCount (CmdbWorkbookBounds.php:207).
appIds are raw cell values, not match keys — lib/Service/Cmdb/CmdbWorkbookReader.php:366. 🟢 Minor — archive.appIds holds raw typed cell values; say so, or normalise Read-only probe: cells 7.0, ' 8 ', 'app-9' come back as 7 (int), ' 8 ' and 'app-9'.
Profile accepts 'archive' while the endpoint still refuses it — lib/Settings/cmdb-import/topdesk-profile.json:48. 🟢 Minor — heads-up: until #1286 merges, the profile and the endpoint disagree The profile now lists ["keep", "archive"] and missingRecordsModes() reads it, but nothing in lib/ calls missingRecordsModes() on this head (only the unit test does): CmdbExportImportService::supportsMissingRecords() is still $mode === 'keep' and CmdbImportController still answers details.accepted: ['keep'].

✅ Fixed before posting (1)

Renamed test is still named by the live spec — fixed in 388181a.

✅ Verified clean

readBounded() refactor: the returned archive array matches the pre-refactor version (softwarecatalog 141ea249) for no sheet, a sheet without the key column and a sheet with it — checked by reading and a read-only probe.
Schema bumps module 0.3.9 / usage 1.5.7 follow the fragment-version convention and are pinned by TopdeskCmdbFragmentTest and PublicationFieldRulesTest.
x-openregister-archive matches OpenRegister's Schema::ARCHIVE_ANNOTATION.

Scope decision: slice 1 of 3 of the closed #1281, 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-031, ADR-032, ADR-037 — all resolve on hydra main.
Jira context: WOO-587 — CMDB-import: gearchiveerde en verdwenen applicaties bij herimport (status: Refinement)

github-actions · 2026-10-12T12:53:10Z

Quality Report — ConductionNL/stackiq @ `de5dedb`

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					✅ 1233/1233 · 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:53 UTC

Download the full PDF report from the workflow artifacts.

The archive sheet is formula-derived. A row whose APPID formula carries no cached value read as an empty row and was skipped without trace, so the sheet reported present: true with one APPID fewer. The reconciliation would then see that application on no sheet and soft-delete it instead of archiving it, with nothing in the report saying why. REQ-CMDB-002 already treats such a cell on the CMDB sheets as empty plus a row warning. readRows() now hands back the numbers of rows that read as empty only because a formula in them is uncached, and the reader returns them as archive.uncachedRows. The spec delta (REQ-CMDB-015, new scenario), design D3/D5 and task 2 state the rule: such a sheet is incomplete, so the import warns naming the sheet and rows and archives every missing application, deleting none. The service side follows in the stacked PR. Review #1285 f1 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

REQ-CMDB-015 and design D3 promise that the archive sheet is read under the same bounds as the CMDB sheets, for its key column only, but only the row limit had a test. The part size, the row span, the referenced-string budget, the key-only read filter and the null cases of archiveSheetName() could regress unnoticed; swapping readSheetNames() for sheetNames() in read() left the reader tests green, which would blank an oversized archive sheet instead of refusing it. Adds one test per bound. RecordingXlsxReader now also keeps the read filter of each load, so the key-only claim is asserted on what the data pass is allowed to read, not on the returned array. Each test was checked by mutating the reader (archive sheet left out of that bound) and goes red. Review #1285 f5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The rule "archive, never soft-delete" was written as "when the workbook has no archive sheet" in the design risk list, the proposal summary and Risk 2, and in task 4. It also applies when the sheet has no APPID column or holds an APPID the reader cannot see (a formula without a cached value). Review pre-push p1 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

D8 said the facet narrowing's _ids composes with the Archived quick filter. The Archived tab of the Applications page overrides _ids with an empty value, so it lists the whole archive and ignores facets and search. Review pre-push p2 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ry import The docs said the import "ignores all other sheets" and that a sheet it does not read may exceed the part size limit. The archive sheet is read (its APPID column only) in every import, also with Keep, so it counts against the row span and part-size limits like a CMDB sheet and can refuse a keep import. The file section, the row limit, the part-size limit and the WORKBOOK_TOO_LARGE row now say so, and that deleting the sheet from a copy makes the import archive and never delete. Review #1285 f2 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-12T16:18:23Z

Quality Report — ConductionNL/stackiq @ `fd8f758`

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					✅ 1239/1239 · 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:18 UTC

Download the full PDF report from the workflow artifacts.

WilcoLouwerse commented Oct 12, 2026

View reviewed changes

WilcoLouwerse and others added 4 commits October 12, 2026 17:04

WilcoLouwerse mentioned this pull request Oct 12, 2026

feat(cmdb-import): run the mapping packs stored in OpenRegister, seeded from the shipped files #1288

Open

Navigation Menu

Repository navigation

feat(cmdb-import): read the archive sheet and let module and usage be archived - #1285

feat(cmdb-import): read the archive sheet and let module and usage be archived#1285
WilcoLouwerse wants to merge 6 commits into
developmentfrom
feat/woo587-cmdb-import-archive-reconciliation-1-reader

WilcoLouwerse commented Oct 12, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 12, 2026

Uh oh!

Uh oh!

Uh oh!

WilcoLouwerse Oct 12, 2026

Uh oh!

Uh oh!

WilcoLouwerse Oct 12, 2026

Uh oh!

WilcoLouwerse Oct 12, 2026

Uh oh!

WilcoLouwerse Oct 12, 2026

Uh oh!

WilcoLouwerse left a comment •

edited

Loading

Uh oh!

github-actions Bot commented Oct 12, 2026

Uh oh!

github-actions Bot commented Oct 12, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

WilcoLouwerse commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Stack

Review fixes (2026-10-12)

Tests

Uh oh!

github-actions Bot commented Oct 12, 2026

Quality Report — ConductionNL/stackiq @ 0c0afb5

Uh oh!

Uh oh!

Uh oh!

WilcoLouwerse Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

WilcoLouwerse Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

WilcoLouwerse Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

WilcoLouwerse Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

WilcoLouwerse left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Verdict: APPROVE (Thorough) — self-review posted as COMMENT (GitHub blocks self-APPROVE)

🟡 Concerns (3)

🟢 Minor (4)

✅ Fixed before posting (1)

✅ Verified clean

Uh oh!

github-actions Bot commented Oct 12, 2026

Quality Report — ConductionNL/stackiq @ de5dedb

Uh oh!

github-actions Bot commented Oct 12, 2026

Quality Report — ConductionNL/stackiq @ fd8f758

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

WilcoLouwerse commented Oct 12, 2026 •

edited

Loading

Quality Report — ConductionNL/stackiq @ `0c0afb5`

WilcoLouwerse left a comment •

edited

Loading

Quality Report — ConductionNL/stackiq @ `de5dedb`

Quality Report — ConductionNL/stackiq @ `fd8f758`