Repository navigation
feat(cmdb-import): archive or soft-delete applications that left the export - #1286
Merged
WilcoLouwerse merged 42 commits intoOct 15, 2026
Conversation
…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>
…uick filter The CMDB import section lets the admin archive or delete applications missing from the export, or keep them, and shows the archived, unarchived, deleted and restored outcomes. The Applications and Applications in use pages get an Archived quick filter. Vitest and the Playwright spec cover it; the feature docs and the changelog describe it. Third of three slices of #1281, on top of the reconciliation slice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 12, 2026
…' 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>
…ile' into feat/woo587-cmdb-import-archive-reconciliation-3-ui
…d to split the export With missingRecords archive as the default, two texts were now wrong: - The section's notes said applications missing from a newer export "are left as they are"; they are archived or deleted unless the admin keeps them. - The TOO_MANY_ROWS, FILE_TOO_LARGE and WORKBOOK_TOO_LARGE hints, in the section and in the feature docs, advised splitting the export and importing the parts one after the other. An archiving import of one part treats the applications of the other parts as missing, so each part archived or deleted the others. The hints now say to import every part with "Keep them as they are". Dutch follows in nl.js and nl.json. Checked with jest (cmdbImport.spec.js, CmdbImport.spec.js: 86 passed), test:l10n and check:l10n-js; eslint could not run locally (Node lacks module.findPackageJSON). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A split export imported with Keep them as they are archives and deletes nothing, so the FILE_TOO_LARGE and TOO_MANY_ROWS rows now say that applications which left the export are then not archived or deleted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WilcoLouwerse
commented
Oct 12, 2026
WilcoLouwerse
commented
Oct 12, 2026
Author
There was a problem hiding this comment.
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)
- An export with no CMDB rows soft-deletes every application —
lib/Service/CmdbExportImportService.php:582. Reconciliation runs whenever the import was not cancelled, with no check on what the file contained. ✅ Answered: intended per WOO-586 Q4-3, no change. - Contract branches of the totality promises have no test —
tests/Unit/Service/CmdbExportImportServiceTest.php:2027. The PR body and the spec make four 'never/every/only' promises. ✅ Addressed ina4570e79. - Reconciliation ignores the working>archived>trash rule for same-key duplicates —
lib/Service/CmdbExportImportService.php:2461. Every other match path prefers working over archived over trash (matchModule, upsertUsage, findInAnyState), because a module deleted by hand next to the one a later import created must not come back. ✅ Addressed inb30cd7f7, d805c5bc.
🟢 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 withoutprevious: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 readsarchived/deletedwith 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
failedrow —lib/Service/CmdbExportImportService.php(reconcileApplication). The application's own usage and module are already written by then, but the report shows only thefailedrow; 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 + 1againstrows + scope. - phpmd refactor (
registerTrash,usagesInEveryState,modulesInChunkwith+=over disjoint chunks,addRunWarnings, controller-sidein_arrayonmissingRecordsModes()): behaviour unchanged; nosupportsMissingRecordscaller 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)
…ng progress In the reconciling phase the service reports processed = rows + index and total = rows + the municipality's applications, so the page showed "410 of 810" as if it were applications missing from the export, and the bar fell from 100% to about 50% when the phase began. The scope also holds applications that are on the sheets and need no change. The progress view now shows no count for that phase, says it checks the applications of the municipality against the export, and keeps the bar full. The l10n key is replaced in place; the docs describe the new progress line. Review #1287 f3 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
After Cancel the section said the import "stops before the next row" and "the rows processed before it stopped are kept". During the comparison it stops before the next application, and the applications archived or deleted until then stay too; the cancelled banner told the admin nothing of that while the report listed them. Both strings now name applications and the l10n keys are replaced in place; the docs say the same. The spec mounts the section for real so the banner and the status line are read from the page. Review #1287 f4 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…' into feat/woo587-cmdb-import-archive-reconciliation-2-reconcile
…the finished sentence The sentence the aria-live region announces (and the report heading shows) only had the row counts, so an import that read 0 rows and archived one application and deleted one was announced as "0 rows read: 0 created, 0 updated, 0 unchanged" and a screen reader user never heard that applications were deleted (WCAG 4.1.3). The sentence now ends with "Applications: ..." listing the non-zero counts, after a cancel too, and nothing when no application was touched. The docs mention it. Review #1287 f5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…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>
…e the facet id set The Applications page narrows its list with `_ids` (the objects a GEMMA facet or the search matched) and merges the active quick filter's map next to it, so the tab's `_archived=true` was ANDed with an id set that FacetService builds from the working set. OpenRegister's default lens leaves archived objects out of that set, so with a facet selected the Archived tab asked for live ids that are archived and listed nothing. The Archived tab's filter now also carries an empty-string `_ids`, which wins over the page's `_ids` in the merge and is left out of the request. An empty array would not do: the filter resolver drops it before the merge. The tab lists the whole archive and ignores the GEMMA selection and the search term; the docs say so. The vitest runs the package's own filter resolver and query builder, and pins that the active tab is merged last. Read in @conduction/nextcloud-vue 2.65.0 (the installed version). Review #1287 f6 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>
…ed-row rule testable The section's tests set `missingRecords` on the component and stubbed the radio away, so `v-model`, `value`, `name` and `:disabled` on the two radios were never exercised, no test clicked "Keep them as they are", and the rule that a deleted application has no link to its module had no test at all. The jest spec now mounts the section with radio and table stubs that render what the page does, clicks Keep and reads the posted form, checks the default, the shared name and the disabled state, and asserts the report table links an archived or failed row and not a deleted one, and shows no row number for a reconciliation row. Each test goes red when the wiring or the rule is mutated. The e2e adds a Keep import that must archive and delete nothing and a check that the archived application is absent from the Applications page before the Archived tab; the spec scenario for the choice now points at it. Review #1287 f8 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… sheets the import reads The WORKBOOK_TOO_LARGE hint said the import "reads only" the two CMDB sheets and told the admin to delete the sheets it does not read; the section's file help said other sheets are ignored. The import also reads the APPID column of "Gearchiveerde Applicaties", and an admin who followed the hint would delete that sheet, so the next import archives and never deletes. Both strings now name the archive sheet and its APPID column (replacing the l10n keys in place), and the spec pins the new hint. Review #1287 f9 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>
…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>
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>
…ile' into feat/woo587-cmdb-import-archive-reconciliation-3-ui
…' into feat/woo587-cmdb-import-archive-reconciliation-2-reconcile
… never deletes The docs and the choice's help said an import only archives when the archive sheet is missing (the docs added "or has no APPID column" and said the result warns the sheet was not found). The service also archives, with its own warning for each case, when the sheet has no APPID column or an APPID in it has no cached value. The docs table row and the section help now name the three cases and say the result warns; the l10n key is replaced in place and a jest case pins the help. Review pre-push p1 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…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>
… merge with the facet ids The manifest note and the view's docblock said a quick filter composes with the facet `_ids` as an additive AND that never clobbers. Since the Archived tab carries an empty-string `_ids`, that tab replaces the facet id set and lists the whole archive, ignoring the GEMMA facets and the search term. Both now name that exception; the docs already said so. A vitest pins the note. Review pre-push p2 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>
…ile' into feat/woo587-cmdb-import-archive-reconciliation-3-ui
The warning has a one-row form and an "in N rows (…)" form that names the first ten rows; the docs quoted only the first. Review pre-push q4 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…' into feat/woo587-cmdb-import-archive-reconciliation-2-reconcile
The help under "Applications missing from the export" was a paragraph of seven lines. It is now one sentence; the full rules, including when applications are only archived, moved to the section's information panel. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…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>
…ile' into feat/woo587-cmdb-import-archive-reconciliation-3-ui
…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>
… loudly on a bad lens read Closes "e2e waits for networkidle, which never settles on Nextcloud (gate-58)": the archive test now waits for a working application row to be visible before asserting that the archived application is absent, so the negative assertion cannot pass on a list that has not rendered. No other networkidle wait remains in the touched spec (git grep over tests/e2e shows only comments explaining why it is avoided). Closes thread 4248187506 (e2e cleanup comment): the header and the helper doc no longer claim that an interrupted run leaves nothing behind or that _includeDeleted adds the trash (a soft-deleted row cannot be listed, design D9); objectsInLens now fails the test on a non-OK response instead of returning an empty list that made cleanup skip silently. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… ignored Closes "Archived tab keeps facet sidebar and search active but ignores them": FacetedCatalogIndexView tracks the active quick-filter tab and shows a note under the toolbar while a tab whose filter carries the empty _ids override is active; en and nl strings and the generated l10n js are added. A mounted view test asserts the note appears on the Archived tab, is gone on another tab, and goes red with the note removed. Closes "Archived-tab facet override is tested against a local merge, not the view": the facet-override test now calls the library's own useSelfFetchList and reads the fixedFilters getter it hands the list composable, instead of restating the spread; only the list composable and store are replaced. It goes red when the _ids override is removed from the manifest. The source grep test that pinned the merge order is dropped as redundant. Class sweep: the Services page uses the same view but has no quick filter with the override, so it shows no note. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…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>
…ile' into feat/woo587-cmdb-import-archive-reconciliation-3-ui
… tab The view started with no known tab and only learned of a change from the tab's click, but it remounts CnIndexPage whenever the facet or search key changes, which puts the page back on its default tab; the note then showed over the default list. The view now starts from the tab the library starts on (first quick filter marked default, else the first) and returns to it whenever the remount key changes. Tests: the note follows the default tab at mount, and disappears when a search change remounts the list while the Archived tab is active; the remount test goes red without the reset. The e2e wait for a listed application is scoped to the Applications list container, so a row of another table cannot satisfy it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…reconciliation-3-ui' into the review fixes # Conflicts: # l10n/en.js # l10n/en.json # l10n/nl.js # l10n/nl.json
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
commented
Oct 13, 2026
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
Author
There was a problem hiding this comment.
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
dca8211and27a2110:- 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 asuncachedKeyRows. - 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
dca8211and27a2110: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, andremoveis 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)…chive-reconciliation-3-ui feat(cmdb-import): offer the missing-records choice and an Archived quick filter
WilcoLouwerse
merged commit Oct 15, 2026
0e4bcd4
into
feat/woo587-cmdb-import-archive-reconciliation-1-reader
2 checks passed
WilcoLouwerse
deleted the
feat/woo587-cmdb-import-archive-reconciliation-2-reconcile
branch
October 15, 2026 08:26
This was referenced Oct 15, 2026
feat(cmdb-import): run the mapping packs stored in OpenRegister, seeded from the shipped files
#1288
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Second of three stacked PRs that replace #1281. With
missingRecordsarchive, the default, a repeated CMDB import now reconciles a municipality's applications with the newer TOPdesk export.archived,unarchived,deletedandrestored. Report rows carry only APPID, name and uuids, never person data.missingRecordsiskeep | archive, defaultarchive;removestays reserved and answers 422. Without OpenRegister's archive services an archiving import answers 503ARCHIVE_UNAVAILABLEbefore 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::convertRowToObjectEntitydoes not hydrate_archived, and it parses the JSON_deletedcolumn as aDateTime, so_includeDeleted=truenever 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 throughMagicMapper::findDeletedAcrossAllMagicTables(). The unit-test doubles model that behaviour.Stack
The repository's quality workflow runs only on PRs into
development,betaormain, so this PR gets its checks once #1285 merges and GitHub retargets it todevelopment.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)
d3601b33f196420b@spectags this PR added point at the archivedcmdb-export-importchange.08acba6d,16de6470b30cd7f7,d805c5bca4570e79e8772a74An 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
🤖 Generated with Claude Code