Repository navigation
feat(cmdb-import): offer the missing-records choice and an Archived quick filter - #1287
Open
WilcoLouwerse wants to merge 24 commits into
Conversation
…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
…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)
Both blockers — the section still told the admin that missing applications stay, and the size hints advised a split import that makes each part archive the others — were fixed on the branch before posting. What remains are wording and test gaps in the UI around the new phase.
Update 2026-10-12: All concerns and the hint minor were fixed on the branch after posting; f7 stays open.
🟡 Concerns (5)
- Reconciliation progress counts rows as applications —
src/utils/cmdbImport.js:955. In phasereconcilingthe service sets total_items = rows + scope and reports processed_items = rows + index + 1 (CmdbExportImportService.php:2198, 2210), so 400 rows and 410 applications show 'Checking the applications missing from the export: 410 of 810' and the bar falls from 100% to about 50%. ✅ Addressed in3671df07. - Cancel texts still say the rows, not the applications —
docs/features/cmdb-import.md:88. The docs now say Cancel stops 'before the next row, or, while the missing applications are compared, before the next application; what was already processed stays imported'. ✅ Addressed inbbf42096. - Finished announcement omits archived and deleted counts —
src/views/settings/sections/CmdbImport.vue:659.finishedText(CmdbImport.vue:672-700) is the sentence the aria-live region announces and the report heading shows: 'Import for X finished. N rows read: C created, U updated, K unchanged.' The new outcomes only get summary tiles. ✅ Addressed inac3305a7. - Archived filter on Applications may combine with the facet id set to nothing —
src/manifest.json:667. The Applications page is FacetedCatalogIndexView, which sends{ _ids: matchedObjectIds }whenever a GEMMA facet or search is active (FacetedCatalogIndexView.vue:317-328) and merges the quick filter on top (docblock lines 225-236: 'logical AND'). ✅ Addressed inf99c0630. - Choice wiring and deleted-link rule have no test that can fail —
tests/e2e/spec-coverage/cmdb-import.spec.ts:563. Audit of the new tests. (a) CmdbImport.spec.js setswrapper.vm.missingRecords = 'keep'directly and stubs NcCheckboxRadioSwitch asrender: () => null, sov-model,value,nameand:disabledon the two radios are never exercised; the spec exclusion for 'The admin chooses to keep missing applications' relies on this test. ✅ Addressed inb1d249a6.
🟢 Minor (2)
- E2E cleanup comment says it reaches the trash —
tests/e2e/spec-coverage/cmdb-import.spec.ts:36. The header (lines 35-37) and the docblock (line 143: '_includeDeletedadds the trash') say the cleanup looks through the trash lens. - Hint says the import reads only the two CMDB sheets —
docs/features/cmdb-import.md:122. The docs now say the import also reads the APPID column of 'Gearchiveerde Applicaties', but the WORKBOOK_TOO_LARGE hint (cmdbImport.js:567) still says 'it reads only "{first}" and "{second}"' and tells the admin to delete the sheets the import does not read; cmdbImport.spec.js:429 pins the old sentence. ✅ Addressed in8a1562ae.
✅ Fixed before posting (2)
- Import notes still say missing applications are left as they are —
src/views/settings/sections/CmdbImport.vue:171. With archiving now the default, the section's notes still said applications missing from a newer export are left as they are; they now say they are archived or deleted unless kept. Fixed inf6ae270. - Split-the-export advice now makes each part archive the other —
docs/features/cmdb-import.md:86. The size hints and docs advised importing a split export part by part, which with archiving makes each part archive or delete the others; they now say to import every part with "Keep them as they are". Fixed inf6ae270.
✅ Verified clean
- Every new string is in
l10n/en.jsonandl10n/nl.json; outcome names andmissingRecordsvalues matchCmdbImportReport, the controller andopenapi.json. ARCHIVE_UNAVAILABLEhas its own title and hint and is treated as the import's answer, not an interruption.- Archived quick filters use the same label, icon and
_archived: "true"on both pages;ArchiveOutlineis registered and pinned.
Scope decision: slice 3 of 3 of the closed #1281, stacked on #1286; 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: 55 of 56 applicable gates green; gate 60 icon-vocabulary did not run locally (no vue-material-design-icons) and passed in CI on the identical src/ at #1281's 141ea249. Conflict markers and PHP syntax green on 97c4e99. jest (86) and the l10n checks pass locally after the fix; eslint could not run locally (Node lacks module.findPackageJSON).
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>
…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>
…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>
…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>
…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>
…ile' into feat/woo587-cmdb-import-archive-reconciliation-3-ui
… 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>
… 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>
…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>
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>
…ile' into feat/woo587-cmdb-import-archive-reconciliation-3-ui
… 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
added a commit
that referenced
this pull request
Oct 13, 2026
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>
WilcoLouwerse
commented
Oct 13, 2026
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 blocker (gate 58), the concern and the minor found this round, and the open minor from round 1, were fixed on the branch before posting. A pre-push check found one more concern and two minors, which were fixed before the push. All 10 threads are resolved. What still stands between this PR and a merge: 1 approval, and its bases #1286 and #1285.
| Round | Found | Status |
|---|---|---|
1 (97c4e99) |
2 🔴 fixed before posting · 5 🟡 (progress counts, cancel texts, finished counts, facet id set, untestable wiring) · 2 🟢 | 🔴 fixed in f6ae270; 🟡 resolved in 3671df0, bbf4209, ac3305a, f99c063, b1d249a; 🟢 resolved in 8a1562a and c55a96d |
2 (a1cca04) — this review |
🔴 gate-58 networkidle · 🟡 Archived tab silently ignores facets and search · 🟢 facet-override test restated the merge |
fixed before posting in c55a96d, 4b70494, 1719b19 |
| Finding | Was | Now | Where |
|---|---|---|---|
| 🟢 E2E cleanup comment says it reaches the trash | 🟢 r1 | resolved in c55a96d |
thread |
🔴 Gate 58 e2e-networkidle: the test waited for networkidle |
new | fixed before posting in c55a96d, tightened in 1719b19 |
thread |
| 🟡 Archived tab keeps facets and search active but ignores them | new | fixed before posting in 4b70494 + 1719b19 |
src/views/FacetedCatalogIndexView.vue |
| 🟢 Facet-override test restated the merge locally | new | fixed before posting in 4b70494 |
tests/vitest/cmdbArchiveQuickFilter.spec.js |
Details of this round
Scope decision: delta 97c4e99…323d761 (10 non-merge commits, 14 files, 801 changed lines; the base merges 226b76c/9a5ffff show no resolved conflict), plus this run's fix commits and merges up to a1cca04. Not reviewed: 8dfa03a (help text of the missing-applications choice shortened), pushed from another session during this review and merged in a1cca04. Its conflict with this run's note key was resolved in all four l10n files, and the JS catalogues were rebuilt with npm run l10n:build.
✅ Fixed before posting (3)
- Gate 58
e2e-networkidle.c55a96dand1719b19: the test assertsworking.modules > 0and waits for.faceted-catalog-index__list tbody trbefore the negativetoHaveCount(0).objectsInLensnow fails on a non-OK response instead of returning []. - Archived tab ignores facets and search silently.
4b70494and1719b19:- A
role="note"paragraph says the tab lists the whole archive and ignores the GEMMA filters and the search term. It has en and nl strings. - The view takes its initial tab the way the library does (the first quick filter marked
default, else the first). A watch onindexPageKeykeeps the note in step with the list when CnIndexPage remounts. - Tests:
tests/vitest/facetedCatalogArchiveNote.spec.js(3). Without the reset, the remount test goes red.
- A
- Facet-override test restated the merge.
4b70494: the test runs the library's realuseSelfFetchListmerge with the shipped manifest, and goes red when_ids: ''is removed.
Docs/tasks:
docs/features/cmdb-import.mdnow names the Archived-tab note and the rule that an unreadable CMDB-sheet APPID archives instead of deleting (#1286).tasks.mdticks Task 6 and Task 7 Implement. Task 7 Test stays open until Playwright has run.
Checks after the last edit:
- Pass: jest 201; the vitest specs for these fixes 8 of 8; eslint, prettier and stylelint on the touched files;
test:l10n;check:l10n-js;check:manifest. - The full vitest suite has 6 specs that fail with and without these changes (
connectionsPages,maintenance,usages,settingsInfoPanels). test:l10n:parityfails with the same 711 lines on this PR and on #1286.- Playwright was not run.
Gates: local run (stacked PR, no CI) at 323d761, 88 of 89 applicable gates ran. Not run: icon-vocabulary. Gates 22, 53 and 116 failed for wiring only (ajv not resolvable), so they count as not run. Gate 58 failed on b1d249a and is fixed above. The gate-16 hit is outside this diff.
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 "waitForLoadState('networkidle')" -- tests/e2e # empty
npx vitest run tests/vitest/facetedCatalogArchiveNote.spec.js tests/vitest/cmdbArchiveQuickFilter.spec.js # 8 greenThis branch has not been deployed
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
Third of three stacked PRs that replace #1281.
_archived=true) on the Applications and Applications in use pages._archived=truefinds the archived ones.docs/features/cmdb-import.mddescribes repeat imports, the choice, the outcomes and the errors; the changelog lists the change.tests/e2e/spec-coverage/cmdb-import.spec.tsimports the archived variant after the anonymised export, then the original again, and checks archived, deleted, unarchived, restored and no duplicates._includeDeleted=true, which runs into the OpenRegister converter bug described in #1286, so soft-deleted test objects may be left behind on the CI instance.Stack
This PR's tree equals #1281's head plus
development's one newer commit. Its checks run once it is retargeted todevelopment.Overlap with the mapping-view PRs (WOO-588)
#1289 and this PR both rewrite the sentence in
src/utils/cmdbImport.jsthat names the sheets the import reads: this PR adds the archive sheet ("… and of the sheet {archive} only the APPID column"), #1289 builds it from the profile's whole sheet list ({sheets}). Whichever merges second keeps #1289's list-based functions (fileHelpText,sheetsReadText,errorText) and adds the archive sheet there, with the l10n keys replaced, not added next to each other.CmdbImport.vue, its spec,l10n/nl.*andopenspec/specs/cmdb-export-import/spec.mdconflict too (keep both sides).Review fixes (2026-10-12)
f6ae27083671df07bbf42096ac3305a7f99c0630,2d288acd_ids, so facets and search do not apply there.b1d249a68a1562ae,cdbec3d3,9fb2dfdb,97c4e999,323d7615Tests
cmdbArchiveQuickFilter.spec.jscmdb-import.spec.ts🤖 Generated with Claude Code