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

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.

… document the reader's bounds Closes the review findings: - "design D8 describes UI code not in this PR": D8 now says the UI code lands in #1287. - tasks.md ticked Implement/Test for Tasks 3-7 whose code is not in this PR: unticked, with a status note that they land in #1286 and #1287. - CmdbWorkbookReader sheet count always includes the archive sheet: documented in the read() docblock (limit / 3 is deliberate). - appIds are raw cell values: documented as raw, untrimmed and un-normalised; no consumer exists in this PR, so no code change. Class sweep: docs/features/cmdb-import.md described the referenced-string bound as "once per CMDB sheet"; it now names the archive sheet too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-13T17:29:24Z

Quality Report — ConductionNL/stackiq @ `3f23a25`

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-13 17:29 UTC

Download the full PDF report from the workflow artifacts.

WilcoLouwerse

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

Nothing left to address: the one new minor and the four open minors from round 1 were fixed on the branch before posting. All 7 threads are resolved. What still stands between this PR and a merge: 1 approval, and the CI run on ae31ac3, which started with this push.

Round	Found	Status
1 (`388181a`)	🟡 uncached archive formulas vanished · 🟡 archive bounds applied to keep · 🟡 'same bounds' tested for 1 of 5 · 🟢 tasks.md ticks · 🟢 string budget · 🟢 raw appIds · 🟢 profile vs endpoint	3 🟡 resolved in `16337d9`/`11d9a23`; 3 🟢 resolved in `ae31ac3`; 1 🟢 needed no change (#1286)
2 (`ae31ac3`) — this review	🟢 design D8 described UI code from #1287	fixed before posting in `ae31ac3`

Finding	Was	Now	Where
🟢 tasks.md ticks tasks whose code is in #1286/#1287	🟢 r1	resolved in `ae31ac3`	thread
🟢 Referenced-string budget shrinks for workbooks without the archive sheet	🟢 r1	resolved in `ae31ac3` (documented; docs copy corrected)	thread
🟢 appIds are raw cell values, not match keys	🟢 r1	resolved in `ae31ac3` (documented)	thread
🟢 Profile accepts 'archive' while the endpoint still refuses it	🟢 r1	no change needed: #1286 makes the endpoint accept it	thread
🟢 Design D8 describes UI code not in this PR	new	fixed before posting in `ae31ac3`	`openspec/changes/cmdb-import-archive-reconciliation/design.md:118`

Details of this round

Scope decision: delta 388181a…70d6f93 (4 commits, 7 files, 281 changed lines), plus the fix commit ae31ac3 written in this run.

✅ Fixed before posting (1)

Design D8 describes UI code not in this PR — ae31ac3: D8 now says the quick filters, the _ids override and the src/icons.js entry land in #1287.

Verified at 70d6f93: 16337d9's $unseen branch is on the live path and covered by testAnUncachedArchiveKeyIsListedNotDropped. The archive-only rule reads the same in proposal, design, tasks and spec.
Gates: CI quality / Hydra Gates ✅ on 70d6f93. CI on ae31ac3 was still running when this verdict was posted. ae31ac3 changes docblock and Markdown text only. phpcs on CmdbWorkbookReader.php is clean; PHPUnit was not run for it.
Jira context: WOO-587, CMDB import: archived and vanished applications on re-import.

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

WilcoLouwerse commented Oct 13, 2026

View reviewed changes

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 7 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

github-actions Bot commented Oct 13, 2026

Uh oh!

WilcoLouwerse left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

github-actions Bot commented Oct 13, 2026

Quality Report — ConductionNL/stackiq @ 3f23a25

Uh oh!

WilcoLouwerse left a comment

Choose a reason for hiding this comment

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

✅ Fixed before posting (1)

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`

Quality Report — ConductionNL/stackiq @ `3f23a25`