Repository navigation
Conversation
- an exported application scheme carries its menu, with each target named by uuid and each parent by technical name, so the scheme can move between instances - the grid views the menu opens travel with the scheme; their data widgets name their table or view by uuid as well - on import, menu items and widgets whose target does not exist here are skipped instead of failing the import - the import request is validated into a ContextScheme at the controller, so a malformed scheme gets a 400 Signed-off-by: Remko <remko@conduction.nl> Assisted-by: ClaudeCode:claude-opus-5-5
rjzondervan
left a comment
rjzondervan
left a comment
There was a problem hiding this comment.
Verdict: Request changes (Thorough)
Export and import of menus and grid views works, and access control holds: an import never attaches a table or view the importer cannot manage or read, and ids from the other instance are never used. One regression blocks: importing a scheme without menuItems now deletes the application's menu. The other findings are about the promise that bad items are "skipped instead of failing", and about tests.
🔴 Blockers
🟡 Concerns
- Reused grid view is not checked against the importer
- Inaccessible targets abort the import instead of being skipped
- Grid view import errors return 500 without rollback
- Grid views outside the menu are not exported
- Grid view import and wiring have no tests
🟢 Minor
- Duplicate technical names fail the whole import
gridViewsentries are not validated inContextScheme
✅ Verified clean
- Foreign ids:
resolveGridTargetsreplaces a widget's target id with the one resolved from its uuid, and drops the widget when there is none. - Imported menu items get the same checks as a normal menu save (
MenuItemInput: http/https plus a host, label/icon length, technical-name pattern). - Parents by technical name: positions counted over surviving items only; a skipped or nested parent promotes its children to top level.
- The import runs inside the controller's transaction; export requires MANAGE and de-duplicates grid views by uuid.
ContextScheme: name, tables shape, list types and icon/description types are checked and tested.
Mode: Thorough · Head: 1ee93397 (the merge from stack/a2-application-overview brought only Playwright and Context.vue fixes; the PR diff is unchanged)
CI: green except Playwright; its 3 failures (application.spec.ts ×2, column-relation) also fail on the base branch. The extra failures on some matrix jobs differ between runs.
Not run: local PHPUnit and mutations; the mutation claims in the test finding come from reading the tests.
…hat cannot be used A scheme without a menu leaves the application's menu alone; only an explicit empty menu clears it. Menu targets the importer cannot manage and widget targets whose rows it cannot read are skipped, an existing grid view is reused only when the importer can manage it and copied otherwise, and a later duplicate technical name gets a derived one. Grid views that are only a resource travel with the export too. Grid view entries are validated, and import errors that the service raises answer 400 or 500 after a rollback. The import wiring is covered by tests through overridable service getters. Signed-off-by: Remko <remko@conduction.nl> Assisted-by: ClaudeCode:claude-opus-5-5
… through the dialog The preview rebuilds the scheme it posts from name, icon, description, tables and nodes only, so an import from the dialog dropped the menu and the grid views of the file, and a grid view the instance did not know yet failed the import with a 404. The dialog now adds both from the uploaded file; a file without a menu still leaves the current menu alone. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko <remko@conduction.nl>
| const contextItem = page.locator('[data-cy="navigationContextItem"]').filter({ hasText: 'importing application' }).first() | ||
| await contextItem.hover() | ||
| await contextItem.getByRole('button', { name: /Actions|Open menu/i }).first().click({ force: true }) | ||
| await page.locator('[data-cy="navigationContextExportSchemeBtn"]').filter({ hasText: 'Import scheme' }).click({ force: true }) |
There was a problem hiding this comment.
🟡 Concern — The new dialog e2e test fails on stable34
Playwright (stable34) is red on 835fd42. Only one test fails: the one this commit adds (application.spec.ts:104). It fails on all 3 attempts with a timeout while waiting for input[type=file] in the import dialog. The page snapshot shows the application's Actions menu still open with "Import scheme" in it, so the dialog never opened. The other 103 stable34 tests pass. The same test passes on stable33, stable35 and master, and a2a1ac9 (which already has the merged base) was green on all four Playwright jobs.
Impact: CI stays red. The test meant to prove that the dialog keeps the menu does not run on one supported version.
Done when:
Playwright (stable34)is green on the head commit, with this test included
| continue; | ||
| } | ||
| $now = (new \DateTime())->format('Y-m-d H:i:s'); | ||
| $localIds[$uuid] = $this->viewService()->importView(null, [ |
There was a problem hiding this comment.
🟢 Minor — Re-importing a scheme leaves the previous copy behind
When the importer cannot manage the existing grid view, a copy with a fresh uuid is created. The original uuid still belongs to the other user, so every later import of the same file creates a new copy. update() then replaces the application's nodes and menu with the new copy, and the earlier copy stays in the database. It belongs to no application and has no table to open it from. On one instance, user B importing user A's export twice leaves one copy behind each time. Consider reusing a copy that this application already holds, or say in the docblock that this is accepted.
rjzondervan
left a comment
rjzondervan
left a comment
There was a problem hiding this comment.
Verdict: Approve (Thorough, round 2)
All 8 round-1 findings are resolved; this round adds 1 🟡 and 4 🟢, none blocking under Thorough, with 5 threads open. Before a merge: the PR's own new e2e test is red on
Playwright (stable34)(not a required check).
| Round | Found | Status |
|---|---|---|
1 (1ee93397) |
🔴 1 (menu wiped) · 🟡 5 (reuse, inaccessible targets, 500 without rollback, export misses grid views, tests) · 🟢 2 | all resolved in a2a1ac96, 835fd429 |
2 (835fd429) — this review |
🟡 1 (e2e red on stable34) · 🟢 4 (NotFoundError 500, two guards untested, leftover copies, dialog always replaces the menu) | open |
Details of this round
- Scope: the two fix commits
a2a1ac96and835fd429. The merges ofstack/a2-application-overview(1241d15b) are the base branch's code and were not reviewed here. - Round-1 checks: each finding's done-when read against the head; the four round-1 mutations each now fail a named test (reuse check, grid views imported before the nodes, the menu passed to
update(), de-duplication by uuid), and the controller's 400 path is tested. - CI: green except
Playwright (stable34): 1 failed (the newapplication.spec.tsdialog test, all 3 attempts, dialog never opens), 103 passed; the same test passes on stable33, stable35 and master, anda2a1ac96was green on all four Playwright jobs. - Not run: PHPUnit locally (no
vendor/, no container); CI PHPUnit is green on all four databases.
Before the next round: this round's findings were mostly a missed sibling or caused by the fix (fix rules on: fix-induced 1 + missed-sibling 2 ≥ new 2), so the next fixes go through the class sweep first:
- swap
canManageNodeByIdandcanReadRowsByElementIdinresolveTargetId→ a test must go red - set the controller default of
$menuItemsback to[]→ a test must go red grep -n NotFoundError lib/Controller/ContextController.php→ acatchwithrollBack()inimportScheme
…l back a missing table The import dialog gets a Menu switch: switched off, the scheme is posted without its menu and the application keeps the current one, and only the grid views that the chosen menu or nodes open are sent. A grid view the importer cannot manage is copied under a uuid derived from its own and the application, so importing the scheme into the application again reuses that copy instead of leaving the previous one behind. A table that is gone during the import now gives a 404 with a rollback instead of a 500. The e2e test opens the dialog with a regular click, as a forced click could land before the actions menu settled. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko <remko@conduction.nl>
| const importResponse = page.waitForResponse(r => r.url().includes('/scheme/import') && r.request().method() === 'POST') | ||
| await dialog.getByRole('button', { name: 'Import' }).click() | ||
| const response = await importResponse | ||
| await expect(dialog).toContainText('Import completed successfully.') |
There was a problem hiding this comment.
🔴 Blocker — The scheme import returns a 500 in CI on two versions
Now that the dialog opens, the import request itself fails in CI some of the time. The cause is not known.
Playwright (stable34), testapplication.spec.ts:104(menu on): attempts 1 and 2 fail at this line with "Could not import scheme due to unknown errors.". Attempt 3 passes. In the retry-1 trace, the preview request returns 200, andPOST /ocs/v2.php/apps/tables/api/2/contexts/7/scheme/importreturns 500 with the generichandleErrorbody ("An unexpected error occurred…"). That body meansContextController::importSchemecaught anOCP\DB\Exception, aMultipleObjectsReturnedExceptionor anInternalError.Playwright (stable33), testapplication.spec.ts:115(menu off): attempt 1 fails the same way, and the retry passes.
The posted body has the expected shape: one node, one menu item and one grid view, all pointing at a fresh random uuid. That means importGridViews takes the $existing === null branch, so the new derived-uuid code is not on this path. The failure sits in the import path of the PR as a whole. 835fd42 never got this far on stable34, because the dialog did not open there. The server log of the failing attempt was not kept: the job's "Show logs" step fails with No such container: nextcloud-e2e-test-server_tables.
One possibility, not verified: the controller wraps the whole import in beginTransaction(), and update() runs atomicRetry inside it for SQLite's "database is locked". A lock conflict with a parallel worker inside the outer transaction cannot be retried there and would surface as this 500. Can you get the exception from the server log (locally, with several Playwright workers, or with a working log step)? If it is a lock error, the fix is in the transaction handling. If it is something else, it is a bug in the import.
Impact: an import through the dialog can fail with "unknown errors" and roll back. The two tests that cover the new dialog stay flaky, and Playwright (stable34) stays red.
Done when:
- The exception behind the 500 is named from a server log, and it is either fixed or shown to be an e2e-environment lock with a fix for it.
- On one head commit,
application.spec.ts:104andapplication.spec.ts:115pass on the first attempt on all four Playwright jobs (no "flaky" entry for either).
| nodes: this.importNodes ? this.previewData.nodes.to : this.previewData.nodes.from, | ||
| } | ||
| // Left out, the import keeps the current menu | ||
| if (this.menuItems && this.importMenu) { |
There was a problem hiding this comment.
🟡 Concern — The PR body no longer matches what the PR does
This commit makes the menu optional and changes the dialog. Three statements in the PR body are now wrong:
- "Menu on import: … The imported menu replaces the application's menu." Since this commit the menu is replaced only when
menuItemsis sent. The dialog leaves it out when the Menu switch is off (this line), and the controller then keeps the current menu (?array $menuItems = null). - "Import: … A grid view whose uuid already exists here is reused, not overwritten." The view is reused only when the importer can manage it. Otherwise it is copied under a uuid derived from the application (
ContextService.php:1140-1150). - "### 🖼️ Screenshots — Backend only, no UI changes." The import dialog gets a Menu switch with a From/To list (
ImportContextSchemePreview.vue:183-208), and the dialog now sends only the grid views in use.
The Tests bullet also leaves out the two Playwright tests.
Impact: reviewers and the changelog read a behaviour that the code does not have: an import that always replaces the menu, and no UI change.
Done when:
- The PR body says when the menu is replaced (only when
menuItemsis sent; the dialog's Menu switch), when a grid view is copied instead of reused, and that the import dialog changed, with a screenshot of the switch. gh pr view 27 -R ConductionNL/tables --json body -q .body | grep -c -e "The imported menu replaces the application's menu." -e "reused, not overwritten" -e "Backend only, no UI changes"returns 0.
| $importUuid = Uuid::v5(Uuid::fromString(Uuid::NAMESPACE_URL), 'tables-context-' . $contextId . '/' . $uuid)->toRfc4122(); | ||
| $existing = $this->viewMapper->findByUuid($importUuid); | ||
| } | ||
| if ($existing !== null && $this->canManageView($existing, $userId)) { |
There was a problem hiding this comment.
🟡 Concern — The manage check on a reused copy can be removed without a test failing
Line 1144 decides whether a view found under the derived uuid is reused. The view is reused only when the importer can manage it, and otherwise a new copy is made. No test covers a derived-uuid view that the importer cannot manage:
testImportCopiesAGridViewTheImporterCannotManage: the lookup of the derived uuid returnsnull.testImportingAgainIntoTheApplicationReusesItsEarlierCopy: the copy (id 66) can be managed.testImportReusesAGridViewTheImporterCanManage: the original can be managed.
If you change line 1144 to if ($existing !== null) {, all three tests stay green. The import would then reuse a view that another member of the application created and that this importer cannot manage. One example: user B's copy, imported again by user C who manages the application through a share. update() then rejects that view as a node with a 403, and a menu item resolves to it through the $gridViewIds shortcut in resolveTargetId, which skips the permission check. AGENTS.md asks for every changed segment to be covered by a test.
Suggested fix: add a test where findByUuid returns a view for both the original uuid and the derived uuid, and canManageNodeById returns false for both. Assert that importView is called once and that the returned id is the new view's.
Done when:
- With line 1144 mutated to
if ($existing !== null) {, a named test intests/unit/Service/ContextServiceTest.php(for exampletestImportCopiesAGridViewWhenItsEarlierCopyBelongsToSomeoneElse) goes red.
| $this->assertSame(['view-uuid' => 66], $ids); | ||
| } | ||
|
|
||
| public function testACopyIntoAnotherApplicationGetsAnotherUuid(): void { |
There was a problem hiding this comment.
🟢 Minor — testACopyIntoAnotherApplicationGetsAnotherUuid runs no production code
The test compares two results of the private test helper copyUuid(), which repeats the formula from ContextService.php:1141. It passes whatever importGridViews does, and it would only fail if Uuid::v5 itself stopped depending on its name. The property it names ("another application gets another uuid") is already pinned by testImportCopiesAGridViewTheImporterCannotManage, which checks the production uuid against copyUuid(7, …). Either remove it, or make it call importGridViews twice, with context 7 and context 8, and compare the uuid values passed to importView.
| $existing = $this->viewMapper->findByUuid($uuid); | ||
| if ($existing !== null && !$this->canManageView($existing, $userId)) { | ||
| $importUuid = Uuid::v5(Uuid::fromString(Uuid::NAMESPACE_URL), 'tables-context-' . $contextId . '/' . $uuid)->toRfc4122(); | ||
| $existing = $this->viewMapper->findByUuid($importUuid); |
There was a problem hiding this comment.
🟢 Minor — Importing a newer scheme again keeps the old copy's grid
Before this commit, every import by someone who cannot manage the original made a fresh copy, with the grid from the file. Now the copy under the derived uuid is reused as it is. Suppose the owner changes the page, exports again, and the other user imports that export into the same application. The application then keeps the old layout and widgets, and nothing tells the user. This matches the existing rule for a view the importer can manage (also reused without update), so it may be intended. If so, the docblock at lines 1124-1128 could say that a reused view, original or copy, is not updated from the file. If not, the reused copy should take the file's grid.
| * @param {object} scheme the scheme the preview built | ||
| * @return {Array<object>} | ||
| */ | ||
| usedGridViews(scheme) { |
There was a problem hiding this comment.
🟢 Minor — gridViews is described as "the views the menu points at", but the dialog now also sends the ones the nodes open
usedGridViews sends the grid views that a menu item or a node opens. With the Menu switch off, it sends only the node ones. The backend handles that (importScheme resolves nodes through $gridViewIds), but the API text still says otherwise. These are the live copies:
lib/Controller/ContextController.php:366:$gridViews Views without a table that the menu points atlib/Model/ContextScheme.php:131:Views without a table that the menu points at; …openapi.json:13903andsrc/types/openapi/openapi.ts:7697(generated from the controller docblock)
Something like "Views without a table that the menu or the nodes point at" in the docblocks, then regenerating OpenAPI, would cover all four.
rjzondervan
left a comment
rjzondervan
left a comment
There was a problem hiding this comment.
Verdict: Request changes (Strict, round 3)
1 new blocker and 2 new concerns, with 8 threads open (1 🔴, 3 🟡, 4 🟢); 3 of the 5 round-2 findings are resolved. Before a merge: the 🔴 and the three 🟡 (blocking under Strict);
Playwright (stable34)andcypress (mysql, master)are red but not required, and the cypress failure is not caused by this PR.
| Round | Found | Status |
|---|---|---|
1 (1ee93397) |
🔴 1 (menu wiped) · 🟡 5 · 🟢 2 | all resolved in a2a1ac96, 835fd429 |
2 (835fd429) |
🟡 1 (e2e red on stable34) · 🟢 4 (NotFoundError 500, two guards untested, leftover copies, dialog always replaces the menu) | 3 resolved in f8ade384; e2e and leftover copies open |
3 (f8ade384) — this review |
🔴 1 (import 500 in CI) · 🟡 2 (PR body, untested manage check) · 🟢 3 (helper-only test, old grid kept, gridViews description) | open |
| Finding | Was | Now | Where |
|---|---|---|---|
| 🟢 NotFoundError from the import ends as a 500 without a rollback | 🟢 round 2 | resolved in f8ade384 |
thread |
| 🟢 Two of the new guards can be broken without a test failing | 🟢 round 2 | resolved in f8ade384 |
thread |
| 🟢 An import from the dialog always replaces the menu | 🟢 round 2 | resolved in f8ade384 |
thread |
| 🟡 The new dialog e2e test fails on stable34 | 🟡 round 2 | still open | thread |
| 🟢 Re-importing a scheme leaves the previous copy behind | 🟢 round 2 | still open (now for a second member only) | thread |
| 🔴 The scheme import returns a 500 in CI on two versions | new | new — 🔴 | comment |
| 🟡 The PR body no longer matches what the PR does | new | new — 🟡 | comment |
| 🟡 The manage check on a reused copy can be removed without a test failing | new | new — 🟡 | comment |
🟢 testACopyIntoAnotherApplicationGetsAnotherUuid runs no production code |
new | new — 🟢 | comment |
| 🟢 Importing a newer scheme again keeps the old copy's grid | new | new — 🟢 | comment |
🟢 gridViews is described as "the views the menu points at" |
new | new — 🟢 | comment |
Details of this round
- Scope: the one commit since round 2,
f8ade384(7 files, +277/-56). Strict this round because the commit changes which grid views an importer may reuse or copy. - Commit-message claims checked: the Menu switch (no
menuItemsin the payload when off, controller defaultnull, menu kept), reuse of the derived copy by the same importer, the 404 with rollback, and the regular click that now opens the dialog all hold. - Still open, what changed:
- e2e on stable34: the dialog now opens (upload, preview 200 and the import POST are in the trace). The job is still red:
context.spec.ts:115"Share context with resources" fails 3× on the toast after editing a context, outside this delta and already flaky on the a1 base; andapplication.spec.ts:104is flaky because of the 500 in the 🔴. - Leftover copies: the same importer now reuses its copy. A second member who manages the application through a share cannot manage the first member's copy, so
freeUuidfinds the derived uuid taken and the new copy gets a random v7 uuid: each of their imports leaves the previous copy behind (unless they own the application).
- e2e on stable34: the dialog now opens (upload, preview 200 and the import POST are in the trace). The job is still red:
- CI on
f8ade384:cypress (mysql, master)fails in every spec'sbeforeEachonCannot read properties of undefined (reading 'forEach')fromapps/viewer/js/viewer-main.mjs(the Viewer app on server master), the same error as onmain, so not this PR.Playwright (stable34): see the 🔴 and the e2e row above. The failing import attempts' server log was not kept (No such container: nextcloud-e2e-test-server_tables). - Gates: local run, 49 of 50 applicable gates ran — not run:
composer-audit. The failing set is the same at835fd42andf8ade38, so none comes from this commit; the remaining ones are on the base or not applicable to a nextcloud/tables fork. - Not run: PHPUnit and the e2e suite locally.
Before the next round: this round's findings were mostly fix-induced or a missed copy (fix rules on: fix-induced 3 + missed-sibling 0 ≥ new 1; copy-missed 2) — the next fixes go through the class sweep and the pre-push check first:
git grep -n 'that the menu points at'→ no hit left (4 now)gh pr view 27 -R ConductionNL/tables --json body -q .body | grep -c -e "The imported menu replaces the application's menu." -e "reused, not overwritten" -e "Backend only, no UI changes"→ 0 (3 now)- change
ContextService.php:1144toif ($existing !== null) {→ a named test inContextServiceTest.phpmust go red
3 participants
An exported application scheme now carries its menu and the pages with widgets the menu opens, so an application can move between instances with its menu intact.
What changes
ContextSchemein the controller; a malformed scheme (no name, wrong types, missingaddTables/modifyTables) returns a 400.menuItemsandgridViewsare added to the import scheme endpoint (OpenAPI and TS types regenerated).ContextSchemeand for exporting and importing the menu and grid views.🖼️ Screenshots
Backend only, no UI changes.
🏁 Checklist
/backport to stableX.X🤖 AI (if applicable)