Skip to content

feat(cmdb-import): archive or soft-delete applications that left the export - #1286

Open
WilcoLouwerse wants to merge 17 commits into
feat/woo587-cmdb-import-archive-reconciliation-1-readerfrom
feat/woo587-cmdb-import-archive-reconciliation-2-reconcile
Open

WilcoLouwerse wants to merge 17 commits into
feat/woo587-cmdb-import-archive-reconciliation-1-readerfrom
feat/woo587-cmdb-import-archive-reconciliation-2-reconcile

Conversation

@WilcoLouwerse

@WilcoLouwerse WilcoLouwerse commented Oct 12, 2026 •

Copy link
Copy Markdown

What

Second of three stacked PRs that replace #1281. With missingRecords archive, the default, a repeated CMDB import now reconciles a municipality's applications with the newer TOPdesk export.

  • Archived application: an application whose APPID left the two CMDB sheets but is listed on the archive sheet gets OpenRegister's archive state, on both the module and the municipality's usage.
  • Gone application: an application listed on no sheet is soft-deleted into OpenRegister's trash, where it can be restored for 30 days.
  • Returning application: an application that comes back is unarchived or restored and then updated as usual, so it is never duplicated. A working match is preferred over an archived one, and an archived one over one in the trash.
  • Missing archive sheet: without the sheet, missing applications are archived only, never deleted, and the report warns about it.
  • Cancel: a cancel is honoured before the reconciliation loads its scope and before each application.
  • Report: four new outcomes: archived, unarchived, deleted and restored. Report rows carry only APPID, name and uuids, never person data.
  • Endpoint: missingRecords is keep | archive, default archive; remove stays reserved and answers 422. Without OpenRegister's archive services an archiving import answers 503 ARCHIVE_UNAVAILABLE before the file is read. OpenAPI and Postman describe both.

How the state of a stored object is read (design D9)

On the current OpenRegister beta (2.1.37), MagicSearchHandler::convertRowToObjectEntity does not hydrate _archived, and it parses the JSON _deleted column as a DateTime, so _includeDeleted=true never returns a deleted row. The import therefore reads state per lens: live (no lens), archived (_archived=true), and deleted from the trash, read once per import through MagicMapper::findDeletedAcrossAllMagicTables(). The unit-test doubles model that behaviour.

Stack

  1. feat(cmdb-import): read the archive sheet and let module and usage be archived #1285: the archive sheet, schemas and profile
  2. This PR → base feat(cmdb-import): read the archive sheet and let module and usage be archived #1285's branch
  3. 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

The repository's quality workflow runs only on PRs into development, beta or main, so this PR gets its checks once #1285 merges and GitHub retargets it to development.

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
d3601b33 A trash listing that throws is no longer cached as an empty trash, so a later row cannot duplicate a soft-deleted module.
f196420b The two @spec tags this PR added point at the archived cmdb-export-import change.
08acba6d, 16de6470 An archive sheet with an APPID without a cached value makes the import archive and never soft-delete, with a warning naming the rows (one-row and "in N rows" forms).
b30cd7f7, d805c5bc The reconciliation acts once per import key on the preferred entry, never restores a stale trashed duplicate, and brings the key's other usages and non-trashed modules into the same state.
a4570e79 Tests for the totality promises (another consumer's trashed usage, cancel during the last row, only what the import created, a failing delete or restore step, more than one page, match preference).
e8772a74 The class docblocks state all three archive-only cases.

An export whose CMDB sheets exist but hold no application archives or soft-deletes every application of the municipality the archive sheet does not list. That is the agreed rule (WOO-586): only a workbook with neither CMDB sheet is refused (NO_SOURCE_SHEET).

Tests

Check Result
PHPUnit CMDB, Settings and Fixtures suites on slices 1 + 2 ✅ 333 tests
phpcs / phpmd / phpstan on slices 1 + 2 ✅ 0 errors

🤖 Generated with Claude Code

…export

With missingRecords archive, the default, a completed import reconciles the
municipality's usages and the modules its import created: an APPID on the
archive sheet is archived, one on no sheet is soft-deleted, and without the
archive sheet missing applications are archived only, with a report
warning. A returning application is unarchived or restored before it is
updated, never duplicated. Object state is read per lens (working, then
_archived=true) and from the trash, because OpenRegister's list search
hydrates no archive marker and drops soft-deleted rows. The endpoint
accepts missingRecords keep or archive and translates ARCHIVE_UNAVAILABLE;
OpenAPI and Postman describe the new values and outcomes.

Second of three slices of #1281, on top of the archive-sheet slice.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WilcoLouwerse and others added 3 commits October 12, 2026 13:42
…' into feat/woo587-cmdb-import-archive-reconciliation-2-reconcile
trash() set the cached trash to empty before it read the listing, so a
listing that threw failed its row and left every later row of the run
matching against an empty trash: a soft-deleted module that returned was
created a second time. The cache is now set only once the listing was
read; the next row reads it again. The missing-archive-services path
keeps caching the empty trash, since a retry cannot succeed there.

Mutation-checked: testAFailedTrashReadIsNotCachedForTheNextRow fails on
the previous trash() (outcomes and read count) and passes on this one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…export-import change

openspec/changes/cmdb-export-import was archived to
openspec/changes/archive/2026-10-05-cmdb-export-import, so the tags this
change added on addRunWarnings() and the module write path named a file
that no longer exists. The older tags in lib/ share the stale path and are
left for a separate sweep.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread lib/Service/CmdbExportImportService.php
Comment thread tests/Unit/Service/CmdbExportImportServiceTest.php
Comment thread lib/Service/CmdbExportImportService.php
Comment thread lib/Service/CmdbExportImportService.php
Comment thread l10n/en.json
Comment thread lib/Service/CmdbExportImportService.php

@WilcoLouwerse WilcoLouwerse left a comment •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

No blockers: the reconciliation scope cannot reach another municipality's usage or a hand-made module, cancel and progress counts hold, and today's phpmd refactor is behaviour-neutral. The concern that decides most is the empty-export case (f2), which is a design question for the author.

Update 2026-10-12: Concerns f3 and f4 were fixed on the branch after posting; f2 is answered by the product owner's rule (WOO-586 Q4-3) and needs no change. The pre-push check of those fixes added two minors (below, no thread).

🟡 Concerns (3)

🟢 Minor (7)

  • Missing archive services: the cause is dropped, and a missing return is silently tolerated — lib/Service/CmdbExportImportService.php:2000. archiveServices() catches the container's Throwable and throws ARCHIVE_UNAVAILABLE without previous: and without a log line, so an admin who sees the 503 cannot tell a missing class from a broken constructor.
  • Eleven of the fifteen new l10n keys have no consumer on this slice — l10n/en.json:1044. Only the MISSING_RECORDS_UNSUPPORTED message, the ARCHIVE_UNAVAILABLE message and the two sheet warnings are used by code in this PR.
  • Every import now reads the instance's whole trash into memory — lib/Service/CmdbExportImportService.php:2136. findDeletedAcrossAllMagicTables(limit: null, offset: null) returns every soft-deleted object of every schema and register of the instance, with its data, in one array; only then are this register's modules and usages kept.
  • Destructive reconciliation rows are the first dropped from the stored report — lib/Service/Cmdb/CmdbImportReport.php:291 (outside the diff, no thread). toStoredArray() keeps failed, then skipped, then rows with a warning, then the rest 'in processing order'.
  • moduleBelongsTo checks another organisation's usage in the working lens only — lib/Service/CmdbExportImportService.php:1246 (outside the diff, no thread). The first leg of moduleBelongsTo() now matches this municipality's usage in any state, but the second leg ('does any other organisation use it') still uses findOne() (working lens).
  • A row can be reported for an application whose own objects did not change — lib/Service/CmdbExportImportService.php (reconcileApplication, itemsOf). When only a stray usage or duplicate module of the key moves, the row still reads archived/deleted with the settled application's uuids; REQ-CMDB-015 says an application already in the asked state is not reported.
  • A failing extra turns the whole application into a failed row — lib/Service/CmdbExportImportService.php (reconcileApplication). The application's own usage and module are already written by then, but the report shows only the failed row; the next import heals it. No test covers a failing extra.

✅ Fixed before posting (2)

  • A failed trash read leaves an empty trash cached, so later rows duplicate — fixed in d3601b3.
  • Two new @SPEC anchors point at the archived change path — fixed in f196420.

✅ Verified clean

  • Scope filter: every usage, trash entries included, is re-checked for this municipality as consumer and for the topdesk:<uuid>: key prefix with a non-empty key.
  • Cancel is honoured before the scope load and before each application; progress counts rows + index + 1 against rows + scope.
  • phpmd refactor (registerTrash, usagesInEveryState, modulesInChunk with += over disjoint chunks, addRunWarnings, controller-side in_array on missingRecordsModes()): behaviour unchanged; no supportsMissingRecords caller left.
  • OpenRegister signatures match development: archive/unarchive, restoreObject, findDeletedAcrossAllMagicTables.

Scope decision: slice 2 of 3 of the closed #1281, stacked on #1285; reviewed against its own base only.
Gates: this stacked PR gets no quality workflow until it targets development; hydra-gates ran locally scoped to its diff: 44 of 44 applicable gates green. Conflict markers and PHP syntax green on f196420.
Kept as is after the pre-push check: a listing that keeps failing now fails each row that needs the trash (fail-closed, instead of the silent duplicate it was); only the two tags this PR added moved to the archived change path — the ~64 older tags are a separate sweep.
ADRs verified: ADR-005, ADR-050 — all resolve on hydra main.
Jira context: WOO-587 — CMDB-import: gearchiveerde en verdwenen applicaties bij herimport (status: Refinement)

WilcoLouwerse and others added 13 commits October 12, 2026 17:05
…' into feat/woo587-cmdb-import-archive-reconciliation-2-reconcile
…le APPIDs

The reader now lists the archive-sheet rows whose APPID formula has no
cached value (archive.uncachedRows). The service treats such a sheet like
an absent one for the choice between archive and delete: an application on
no readable sheet may be the one that row holds, so it is archived, never
soft-deleted, and the report carries a warning naming the sheet and the
rows (cut after ten). Applications the sheet does list are archived as
before; keep is unaffected. Without this a workbook saved without cached
values turned every archived application into a deletion, unannounced.

The warning is translated (en, nl) and the OpenAPI description of
missingRecords names the extra case.

Review #1285 f1

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The reconciliation listed every usage of the municipality, so a stale
module deleted by hand next to the one a later import created got its own
scope entry next to the working one. With the key on the archive sheet the
trashed pair was restored and archived: a long-deleted copy of the
application appeared in the archive. Every other match path (matchModule,
upsertUsage, findInAnyState) prefers working over archived over trash.

reconciliationScope() now keeps one entry per key through onePerKey(): the
best module state wins, the best usage state breaks a tie, the first
listed the rest. The other entries are left as they are. REQ-CMDB-015, the
design D5 text and task 4 name the duplicate case.

Review #1286 f4

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
REQ-CMDB-015 promises that only this municipality's usages and what its
import created are reconciled, that each application has its own error
boundary, that a cancel before the scope is loaded reads nothing, and that
the match never duplicates (working before archived before deleted). Most
of these branches had no test that a mutation could turn red; the first
one, the consumer re-check on a trash listing that is not filtered by
consumer, guarded the only thing between another municipality's module and
the trash.

Adds tests for: a trashed usage of another consumer; modules with no
external key, a bare prefix or another origin; a delete that OpenRegister
refuses and a restore that throws, each with a following application still
reconciled; more than one page of usages and one chunk of modules; archived
beating deleted and working beating archived in the match; and an archived
match failing its row under keep when the archive services are missing
(design D7). The cancel test now also asserts the trash was not read and
the usages were not paged. Each test was checked by mutating the service
and goes red.

Review #1286 f3

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The archive-sheet and per-key fixes lengthened a few docblock lines and
the new warning past the 150-character limit, and stateRank() used an
inline if. Names the two array shapes once as phpstan types, shortens the
warning (en, nl, test) and spells the rank out. No behaviour change.

Follows Review #1285 f1 and Review #1286 f4; fold into those commits when
squashing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…' into feat/woo587-cmdb-import-archive-reconciliation-2-reconcile
…cblocks

The class docblock and the addRunWarnings() docblock still named only a
missing archive sheet as the reason to archive instead of delete; the rule
also covers a sheet without an APPID column and one with an APPID that has
no cached value.

Review pre-push p1

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The warning said "in row 2, 3, 4, …" for any number of rows and, once the
list was cut after ten, did not say how many rows there were. It now uses a
singular form for one row and "in N rows (first ten, …)" otherwise, with the
total count; en and nl, and the test covers one, a few and a cut list.

Review pre-push p5

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
onePerKey() reconciled only the best entry per import key and left the
usages of the dropped entries untouched. A module trashed by hand while its
usage stayed working, next to a working pair with the same key, kept that
stray usage visible after the key left the export; so did a second usage of
one module, and a working duplicate module.

The dropped entries now travel as extras of the application: their usages,
and their modules when those are not in the trash, are archived or
soft-deleted with it, once, in the same error boundary and report row.
Objects in the trash are still never restored for an extra, so a stale
duplicate does not come back into the archive. REQ-CMDB-015, design D5 and
task 4 say so.

Review pre-push p4

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…' into feat/woo587-cmdb-import-archive-reconciliation-2-reconcile
…able; usages before modules

Closes the review findings:
- "Unreadable CMDB-sheet APPID still soft-deletes": a CMDB-sheet row whose APPID is a formula without a cached value is skipped as empty, so its application looked absent. The run now notes such rows per sheet and, with any, archives instead of soft-deleting and warns naming the CMDB sheet and the rows (reusing the archive-sheet warning). New test testAnUncachedCmdbRowArchivesInsteadOfDeleting.
- "Extras written after the module; no failing-extra test": itemsOf() now orders every usage (own and extra) before every module, as the docblock promises. New test testAFailingExtraUsageFailsTheApplicationBeforeItsModule.
- archiveServices() swallowed the container exception: it is now logged (exception class only) and chained as previous.
- trash() read for missingRecords=keep: kept on purpose (matching is mode independent, a hand-deleted application must be restored, not duplicated); the reason is now in its docblock.

Class sweep: the rule is updated in the service docblocks, openapi.json (missingRecords description), and the change's spec.md (requirement and new scenario), design.md (D3 text, D5 table, risks) and proposal.md. docs/features/cmdb-import.md does not describe archive reconciliation yet, so it holds no copy of the rule.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Closes the merge artefact from the base fix, which unticked Tasks 3-7 and said their code lands in #1286/#1287:
- Tasks 3, 4 and 5 (matching, reconciliation, controller option) are implemented and tested here: Implement and Test ticked again.
- Tasks 6 and 7 stay unticked: the UI is #1287, and the docs and the e2e run of task 7 are not done.
- The status line now says what is true for this PR.
- Task 4's acceptance criterion also names the CMDB-sheet unreadable APPID case.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…describe what a failed application leaves

Closes the pre-push findings:
- "Fully-empty CMDB row with an uncached APPID stays invisible": the reader dropped a CMDB row whose only content is an APPID formula without a cached value before the service could see it, so its application could still be soft-deleted. read() now returns `uncachedKeyRows` (source sheet => dropped row numbers, collected only when the key column is the uncached one), and the service merges them with the rows that carry `uncached` into the same unreadable-key bookkeeping, so the run archives instead of deleting and warns. New tests: testAnEmptyCmdbRowWithAnUncachedKeyIsListed (reader) and testAFullyEmptyCmdbRowWithAnUncachedKeyArchivesInsteadOfDeleting (service).
- "Failure-branch docblock overstates": the itemsOf() and reconcileApplication() docblocks and design.md D5 and Risks now state the order (own usage, extra usages, module, extra modules) and what a failure leaves: a failing extra usage leaves the own usage changed, that extra visible and the module unchanged, and the row only says failed.

Class sweep: other callers of readRows() are the archive sheet (unchanged, no key filter) and readSourceRows(); the read() docblock names the new key. The spec and proposal speak of "a row of a CMDB sheet" in general and need no change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WilcoLouwerse added a commit that referenced this pull request Oct 13, 2026
… 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>
WilcoLouwerse added a commit that referenced this pull request Oct 13, 2026
…e and unreadable CMDB APPIDs

Makes tasks.md true for this PR after the merge of the reconcile base:
the status line names #1286 for tasks 1-5 and this PR for task 6 (ticked,
with the new Archived-tab note as an acceptance line); task 7 has its
implementation ticked and its Playwright run still open.

docs/features/cmdb-import.md already described archive and soft delete (the
old "left as they are" sentence only remains for the Keep choice); it now
also says that an APPID without a cached value on a CMDB sheet archives
instead of deleting, and that the Archived tab shows a note about the
ignored facets and search.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Nothing left to address: the new concern, the new minor and the two open minors from round 1 were fixed on the branch before posting. A pre-push check found two more minors, which were fixed before the push. All 6 threads are resolved. What still stands between this PR and a merge: 1 approval, and its base #1285.

Round Found Status
1 (f196420) 🟡 export with no CMDB rows soft-deleted everything · 🟡 totality promises untested · 🟡 same-key rule ignored · 🟢 archive-service cause dropped · 🟢 unused l10n keys · 🟢 whole trash read 3 🟡 resolved: one by the author's reply (checked against the WOO-586 plan), two in a4570e7 and b30cd7f/d805c5b; 2 🟢 resolved in dca8211; 1 🟢 consumed by #1287
2 (27a2110) — this review 🟡 unreadable CMDB-sheet APPID still soft-deleted · 🟢 extras written after the module fixed before posting in dca8211, 27a2110
Finding Was Now Where
🟢 Missing archive services: the cause is dropped 🟢 r1 resolved in dca8211 (warning with the class, previous: chained) thread
🟢 Every import now reads the instance's whole trash 🟢 r1 kept on purpose (REQ-CMDB-016), documented in dca8211 thread
🟢 Eleven of the fifteen new l10n keys have no consumer 🟢 r1 consumed by #1287 thread
🟡 Unreadable CMDB-sheet APPID still soft-deletes new fixed before posting in dca8211 + 27a2110 lib/Service/CmdbExportImportService.php (reconcileApplication)
🟢 Extras written after the module; no failing-extra test new fixed before posting in dca8211 + 27a2110 lib/Service/CmdbExportImportService.php (itemsOf)
Details of this round

Scope decision: delta f196420…d805c5b (7 non-merge commits, 10 files, 662 changed lines; the base merges 8360c6a/2e89a1e were read with git show --cc, and the one resolved hunk, in tasks.md, is clean), plus this run's fix commits dca8211, 18bd222, 27a2110 and the merge of #1285's ae31ac3.

✅ Fixed before posting (2)

  • Unreadable CMDB-sheet APPID still soft-deletes. Fixed in dca8211 and 27a2110:
    • A CMDB-sheet row whose key column is an uncached formula used to be skipped, so its application counted as "on no sheet" and was soft-deleted.
    • Such rows now go into the decision as unreadableKeyRows. That includes a row whose only content is that formula, which the reader collects as uncachedKeyRows.
    • With any such row present, the run archives instead of deleting and warns, naming the sheet and the rows.
    • Tests: testAnUncachedCmdbRowArchivesInsteadOfDeleting, testAFullyEmptyCmdbRowWithAnUncachedKeyArchivesInsteadOfDeleting, testAnEmptyCmdbRowWithAnUncachedKeyIsListed.
    • The rule is updated in the docblocks, openapi.json, spec REQ-015 (a new scenario), design D3/D5/Risks and the proposal.
  • Extras written after the module. Fixed in dca8211 and 27a2110:
    • itemsOf() now orders own usage, extra usages, module, extra modules.
    • The docblocks and design D5/Risks say what a failing extra leaves behind.
    • Test: testAFailingExtraUsageFailsTheApplicationBeforeItsModule.

✅ Verified clean

  • reconcileApplication() is the only DELETE decision, and remove is refused. Only an uncached key column blocks a delete; an uncached other column does not.
  • Another municipality's usages never reconcile: testATrashedUsageOfAnotherConsumerNeverReconcilesItsModule.
  • onePerKey() keeps one application per import key and never restores trashed duplicates.

Gates: local run (stacked PR, no CI) at d805c5b, 88 of 89 applicable gates ran. Not run: icon-vocabulary (vue-material-design-icons not installed). Gates 22, 53 and 116 failed for wiring only (ajv not resolvable in the worktree), so they count as not run. The gate-16 hit is on lib/Command/ReferencesAuditCommand.php, outside this diff.
Checks after the fixes: phpunit --filter Cmdb 253 tests, 0 failures, 0 skipped. phpcs and phpstan are clean on CmdbExportImportService.php and CmdbWorkbookReader.php. Mutation tests were not run.
Jira context: WOO-587, CMDB import: archived and vanished applications on re-import.

Before the next round: this round's findings came mostly from the fixing. The next round's fixes go through the class sweep and the pre-push check first.

git grep -n 'unreadableKeyRows\|uncachedKeyRows' -- lib tests   # reader, service and three tests
vendor/bin/phpunit --filter Cmdb                                # 253 green (needs OPENREGISTER_DIR for the workbook tests)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant