From ea32c8203e8f13e2c671d874835ba329540bfe6b Mon Sep 17 00:00:00 2001 From: Remko Date: Wed, 14 Oct 2026 10:40:49 +0200 Subject: [PATCH 1/4] feat(contexts): export and import menus and grid views - 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 Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/Controller/ContextController.php | 12 +- lib/Model/ContextScheme.php | 55 +++++ lib/Service/ContextService.php | 268 ++++++++++++++++++++-- openapi.json | 51 ++++ src/types/openapi/openapi.ts | 21 ++ tests/unit/Model/ContextSchemeTest.php | 66 ++++++ tests/unit/Service/ContextServiceTest.php | 134 +++++++++++ 7 files changed, 587 insertions(+), 20 deletions(-) create mode 100644 tests/unit/Model/ContextSchemeTest.php diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index fe821a9cb9..9d363a87b3 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -16,6 +16,7 @@ use OCA\Tables\Errors\NotFoundError; use OCA\Tables\Errors\PermissionError; use OCA\Tables\Middleware\Attribute\RequirePermission; +use OCA\Tables\Model\ContextScheme; use OCA\Tables\Model\MenuItemInput; use OCA\Tables\ResponseDefinitions; use OCA\Tables\Service\ColumnService; @@ -361,6 +362,8 @@ public function previewSchemeChanges(int $contextId, array $updateScheme): DataR * @param string $description Description of the context * @param list $nodes Ordered meta data of the related nodes * @psalm-param array{addTables: list, modifyTables: list} $tables Tables to be added or modified + * @psalm-param list $menuItems Menu of the context, targets by uuid, parents by technical name + * @psalm-param list> $gridViews Views without a table that the menu points at * * @return DataResponse, array{}>|DataResponse * @@ -371,10 +374,15 @@ public function previewSchemeChanges(int $contextId, array $updateScheme): DataR */ #[NoAdminRequired] #[RequirePermission(permission: Application::PERMISSION_MANAGE, typeParam: 'context', idParam: 'contextId')] - public function importScheme(int $contextId, string $name, string $iconName, string $description, array $nodes, array $tables): DataResponse { + public function importScheme(int $contextId, string $name, string $iconName, string $description, array $nodes, array $tables, array $menuItems = [], array $gridViews = []): DataResponse { + try { + $scheme = ContextScheme::createFromInputArray(['name' => $name, 'icon' => $iconName, 'description' => $description, 'nodes' => $nodes, 'tables' => $tables, 'menuItems' => $menuItems, 'gridViews' => $gridViews]); + } catch (InvalidArgumentException $e) { + return $this->handleBadRequestError(new BadRequestError($e->getMessage(), $e->getCode(), $e)); + } try { $this->db->beginTransaction(); - $context = $this->contextService->importScheme($contextId, $name, $iconName, $description, $nodes, $tables, $this->userId); + $context = $this->contextService->importScheme($contextId, $scheme, $this->userId); $this->db->commit(); return new DataResponse($context->jsonSerialize()); } catch (\InvalidArgumentException $e) { diff --git a/lib/Model/ContextScheme.php b/lib/Model/ContextScheme.php index 3c511632c4..51e29bd141 100644 --- a/lib/Model/ContextScheme.php +++ b/lib/Model/ContextScheme.php @@ -7,6 +7,7 @@ namespace OCA\Tables\Model; +use InvalidArgumentException; use JsonSerializable; class ContextScheme implements JsonSerializable { @@ -18,9 +19,47 @@ public function __construct( protected ?array $nodes = [], protected ?array $pages = [], protected ?array $tables = [], + protected array $menuItems = [], + protected array $gridViews = [], ) { } + /** + * Builds the scheme from an import request and refuses what the import could not handle. + * + * @param array $data + * @throws InvalidArgumentException + */ + public static function createFromInputArray(array $data): self { + $tables = $data['tables'] ?? null; + if (!is_array($tables) || !isset($tables['addTables'], $tables['modifyTables']) || !is_array($tables['addTables']) || !is_array($tables['modifyTables'])) { + throw new InvalidArgumentException('Invalid tables structure provided: expected addTables and modifyTables lists.'); + } + foreach (['nodes', 'menuItems', 'gridViews'] as $list) { + if (isset($data[$list]) && !is_array($data[$list])) { + throw new InvalidArgumentException('The scheme property "' . $list . '" must be a list.'); + } + } + if (!is_string($data['name'] ?? null) || trim($data['name']) === '') { + throw new InvalidArgumentException('A scheme needs a name.'); + } + foreach (['icon', 'description'] as $optional) { + if (isset($data[$optional]) && !is_string($data[$optional])) { + throw new InvalidArgumentException('The scheme property "' . $optional . '" must be a string.'); + } + } + return new self( + name: $data['name'], + icon: $data['icon'] ?? null, + description: $data['description'] ?? null, + nodes: array_values($data['nodes'] ?? []), + pages: [], + tables: $tables, + menuItems: array_values($data['menuItems'] ?? []), + gridViews: array_values($data['gridViews'] ?? []), + ); + } + public function getName(): ?string { return $this->name ?? ''; } @@ -45,6 +84,20 @@ public function getTables(): ?array { return $this->tables; } + /** + * Menu items point at their target by uuid, so a scheme can be imported on another instance. + */ + public function getMenuItems(): array { + return $this->menuItems; + } + + /** + * Views without a table that the menu points at; they live outside the tables of the scheme. + */ + public function getGridViews(): array { + return $this->gridViews; + } + public function jsonSerialize(): mixed { return [ 'name' => $this->name, @@ -53,6 +106,8 @@ public function jsonSerialize(): mixed { 'nodes' => $this->nodes, 'pages' => $this->pages, 'tables' => $this->tables, + 'menuItems' => $this->menuItems, + 'gridViews' => $this->gridViews, ]; } } diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index a10be622b9..3d0c7bc7e7 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -22,6 +22,7 @@ use OCA\Tables\Db\PageMapper; use OCA\Tables\Db\Table; use OCA\Tables\Db\TableMapper; +use OCA\Tables\Db\View; use OCA\Tables\Db\ViewMapper; use OCA\Tables\Errors\BadRequestError; use OCA\Tables\Errors\InternalError; @@ -955,6 +956,8 @@ public function getScheme(int $contextId, string $userId): ContextScheme { } } + [$menuItems, $gridViews] = $this->exportMenuItems($context); + return new ContextScheme( $context->getName(), $context->getIcon(), @@ -962,9 +965,247 @@ public function getScheme(int $contextId, string $userId): ContextScheme { array_values($nodes), $context->getPages(), array_values($tables), + $menuItems, + $gridViews, ); } + /** + * The technical name of a menu item's parent, which names the parent in an export. + * + * @param array{parentId?: int|null} $item + */ + private function parentTechnicalName(Context $context, array $item): ?string { + $parentId = $item['parentId'] ?? null; + if ($parentId === null) { + return null; + } + foreach ($context->getMenuItems() as $candidate) { + if (($candidate['id'] ?? null) === $parentId) { + return $candidate['technicalName'] ?? null; + } + } + return null; + } + + /** + * Menu items with their targets as uuids, plus the views without a table they point at. + * + * @return array{0: list>, 1: list>} + */ + private function exportMenuItems(Context $context): array { + $menuItems = []; + $gridViews = []; + foreach ($context->getMenuItems() as $item) { + $exported = [ + 'label' => $item['label'], + 'icon' => $item['icon'], + 'targetType' => $item['targetType'], + 'targetUuid' => null, + 'url' => $item['url'], + 'technicalName' => $item['technicalName'], + 'parentTechnicalName' => $this->parentTechnicalName($context, $item), + ]; + try { + if ($item['targetType'] === MenuItem::TARGET_TABLE && $item['targetId'] !== null) { + $exported['targetUuid'] = $this->tableMapper->find($item['targetId'])->getUuid(); + } elseif ($item['targetType'] === MenuItem::TARGET_VIEW && $item['targetId'] !== null) { + $view = $this->viewMapper->find($item['targetId']); + $exported['targetUuid'] = $view->getUuid(); + if ($view->isStandalone() && !isset($gridViews[$view->getUuid()])) { + $gridViews[$view->getUuid()] = $this->exportGridView($view); + } + } + } catch (DoesNotExistException|MultipleObjectsReturnedException|Exception $e) { + $this->logger->warning('Menu item target could not be exported, the item is skipped', ['exception' => $e]); + continue; + } + $menuItems[] = $exported; + } + return [$menuItems, array_values($gridViews)]; + } + + /** + * A view without a table, whose data widgets name their target by uuid as well as by id. + * + * @return array + */ + private function exportGridView(View $view): array { + $grid = $view->getGridArray(); + foreach ($grid['widgets'] as &$widget) { + $target = $this->widgetTarget($widget['content'] ?? null); + if ($target === null) { + continue; + } + try { + $target['uuid'] = $target['type'] === MenuItem::TARGET_VIEW + ? $this->viewMapper->find($target['id'])->getUuid() + : $this->tableMapper->find($target['id'])->getUuid(); + } catch (DoesNotExistException|MultipleObjectsReturnedException|Exception) { + $target['uuid'] = null; + } + $widget['content']['target'] = $target; + } + unset($widget); + return [ + 'uuid' => $view->getUuid(), + 'title' => $view->getTitle(), + 'emoji' => $view->getEmoji(), + 'description' => $view->getDescription(), + 'type' => $view->getTypeOrDefault(), + 'createdAt' => $view->getCreatedAt(), + 'lastEditAt' => $view->getLastEditAt(), + 'grid' => $grid, + ]; + } + + /** + * The table or view a data widget shows. + * + * @return array{type: string, id: int}|null + */ + private function widgetTarget(mixed $content): ?array { + if (!is_array($content)) { + return null; + } + $target = $content['target'] ?? null; + if (is_array($target) && is_string($target['type'] ?? null) && is_int($target['id'] ?? null)) { + return ['type' => $target['type'], 'id' => $target['id']]; + } + return null; + } + + /** + * Points the data widgets of an imported grid at the local tables and views by uuid. A widget + * whose target does not exist here is dropped together with its place in the layout. + * + * @return array{widgets: list>, layout: list>} + */ + private function resolveGridTargets(mixed $grid): array { + $widgets = is_array($grid) && is_array($grid['widgets'] ?? null) ? $grid['widgets'] : []; + $layout = is_array($grid) && is_array($grid['layout'] ?? null) ? $grid['layout'] : []; + $resolvedWidgets = []; + $droppedIds = []; + foreach ($widgets as $widget) { + if (!is_array($widget)) { + continue; + } + $target = is_array($widget['content'] ?? null) ? ($widget['content']['target'] ?? null) : null; + if (is_array($target)) { + $targetId = is_string($target['type'] ?? null) && is_string($target['uuid'] ?? null) + ? $this->resolveTargetId($target['type'], $target['uuid']) + : null; + if ($targetId === null) { + $droppedIds[] = $widget['id'] ?? null; + $this->logger->info('Grid widget skipped on import, its table or view does not exist here', ['widget' => $widget['id'] ?? null]); + continue; + } + $widget['content']['target'] = ['type' => $target['type'], 'id' => $targetId]; + } + $resolvedWidgets[] = $widget; + } + $resolvedLayout = array_values(array_filter( + $layout, + static fn (mixed $item): bool => is_array($item) && !in_array($item['widgetId'] ?? null, $droppedIds, true), + )); + return ['widgets' => $resolvedWidgets, 'layout' => $resolvedLayout]; + } + + /** + * Creates the views without a table of a scheme that do not exist here yet. + * + * @param list> $gridViews + * @throws InternalError|BadRequestError + */ + private function importGridViews(array $gridViews, string $userId): void { + $viewService = \OCP\Server::get(ViewService::class); + foreach ($gridViews as $viewData) { + if (!isset($viewData['uuid'], $viewData['title']) || !is_string($viewData['uuid']) || $this->viewMapper->findByUuid($viewData['uuid']) !== null) { + continue; + } + $now = (new \DateTime())->format('Y-m-d H:i:s'); + $viewService->importView(null, [ + 'uuid' => $viewData['uuid'], + 'title' => $viewData['title'], + 'emoji' => $viewData['emoji'] ?? null, + 'description' => $viewData['description'] ?? '', + 'type' => View::TYPE_GRID, + 'createdAt' => $viewData['createdAt'] ?? $now, + 'lastEditAt' => $viewData['lastEditAt'] ?? $now, + 'columnSettings' => [], + 'sort' => [], + 'filter' => [], + 'grid' => $this->resolveGridTargets($viewData['grid'] ?? null), + ], $userId); + } + } + + /** + * The local id of a table or view a scheme refers to by uuid, or null when it does not exist here. + */ + private function resolveTargetId(string $targetType, ?string $uuid): ?int { + if ($uuid === null || $uuid === '') { + return null; + } + try { + if ($targetType === MenuItem::TARGET_TABLE) { + return $this->tableMapper->findByUuid($uuid)->getId(); + } + if ($targetType === MenuItem::TARGET_VIEW) { + return $this->viewMapper->findByUuid($uuid)?->getId(); + } + } catch (DoesNotExistException|MultipleObjectsReturnedException|Exception) { + return null; + } + return null; + } + + /** + * Menu items of a scheme with their uuid targets turned into local ids. Items that are + * invalid, or whose target does not exist here, are skipped. + * + * @param list> $menuItems + * @return list + */ + private function resolveMenuItems(array $menuItems): array { + $prepared = []; + foreach ($menuItems as $item) { + if (!is_array($item)) { + continue; + } + $targetType = $item['targetType'] ?? MenuItem::TARGET_URL; + if ($targetType !== MenuItem::TARGET_URL) { + $item['targetId'] = is_string($targetType) && is_string($item['targetUuid'] ?? null) + ? $this->resolveTargetId($targetType, $item['targetUuid']) + : null; + } + $parentTechnicalName = is_string($item['parentTechnicalName'] ?? null) ? $item['parentTechnicalName'] : null; + unset($item['targetUuid'], $item['parentTechnicalName'], $item['parentIndex']); + try { + MenuItemInput::createFromInputArray($item); + } catch (InvalidArgumentException $e) { + $this->logger->info('Menu item skipped on import: {reason}', ['reason' => $e->getMessage()]); + continue; + } + $prepared[] = ['item' => $item, 'parent' => $parentTechnicalName]; + } + + // A child names its parent by technical name; the parent must have survived the import and be top-level + $parentPositions = []; + foreach ($prepared as $position => $entry) { + $technicalName = $entry['item']['technicalName'] ?? null; + if ($entry['parent'] === null && is_string($technicalName) && !isset($parentPositions[$technicalName])) { + $parentPositions[$technicalName] = $position; + } + } + $items = []; + foreach ($prepared as $entry) { + $entry['item']['parentIndex'] = $entry['parent'] === null ? null : ($parentPositions[$entry['parent']] ?? null); + $items[] = $entry['item']; + } + return MenuItemInput::createListFromInputArray($items); + } + /** * @param int $contextId * @param array $updateScheme @@ -1035,15 +1276,8 @@ public function compareSchemeChanges(int $contextId, array $updateScheme, string } /** - * @param int $contextId - * @param string $name - * @param string $iconName - * @param string $description - * @param array $nodes - * @param array $tables - * @param string $userId - * - * @return Context + * Imports a scheme into an existing context: creates or modifies its tables, resolves the + * nodes and menu targets by uuid, imports the grid views and updates the context itself. * * @throws BadRequestError * @throws ContainerExceptionInterface @@ -1055,12 +1289,9 @@ public function compareSchemeChanges(int $contextId, array $updateScheme, string * @throws NotFoundExceptionInterface * @throws PermissionError */ - public function importScheme(int $contextId, string $name, string $iconName, string $description, array $nodes, array $tables, string $userId): Context { - // Validate the structure of the columns and views arrays - if (!isset($tables['addTables']) || !is_array($tables['addTables']) - || !isset($tables['modifyTables']) || !is_array($tables['modifyTables'])) { - throw new BadRequestError('Invalid tables structure provided.'); - } + public function importScheme(int $contextId, ContextScheme $scheme, string $userId): Context { + $tables = $scheme->getTables() ?? ['addTables' => [], 'modifyTables' => []]; + $nodes = $scheme->getNodes() ?? []; $tableService = \OCP\Server::get(TableService::class); $viewService = \OCP\Server::get(ViewService::class); @@ -1095,6 +1326,9 @@ public function importScheme(int $contextId, string $name, string $iconName, str $tableService->updateTableStructure($table->getId(), $tableScheme['columns'], $tableScheme['views'], $tableScheme['columnOrder'], $tableScheme['sort'], $userId); } + // Grid views first, as nodes and menu items may point at them + $this->importGridViews($scheme->getGridViews(), $userId); + // Resolve node ids for the new tables and views $resolvedNodes = []; foreach ($nodes as $node) { @@ -1118,8 +1352,6 @@ public function importScheme(int $contextId, string $name, string $iconName, str } } - $context = $this->update($contextId, $userId, $name, $iconName, $description, $resolvedNodes); - - return $context; + return $this->update($contextId, $userId, $scheme->getName(), $scheme->getIcon() ?? '', $scheme->getDescription() ?? '', $resolvedNodes, $this->resolveMenuItems($scheme->getMenuItems())); } } diff --git a/openapi.json b/openapi.json index ed6d4e3c59..9021871df1 100644 --- a/openapi.json +++ b/openapi.json @@ -13855,6 +13855,57 @@ } } } + }, + "menuItems": { + "type": "array", + "default": [], + "description": "Menu of the context, targets by uuid, parents by technical name", + "items": { + "type": "object", + "required": [ + "label", + "targetType" + ], + "properties": { + "label": { + "type": "string" + }, + "icon": { + "type": "string" + }, + "targetType": { + "type": "string", + "enum": [ + "view", + "table", + "url" + ] + }, + "targetUuid": { + "type": "string" + }, + "url": { + "type": "string" + }, + "technicalName": { + "type": "string" + }, + "parentTechnicalName": { + "type": "string" + } + } + } + }, + "gridViews": { + "type": "array", + "default": [], + "description": "Views without a table that the menu points at", + "items": { + "type": "object", + "additionalProperties": { + "type": "object" + } + } } } } diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index c5685ec931..01a2d5d330 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -7679,6 +7679,27 @@ export interface operations { readonly addTables: readonly components["schemas"]["Table"][]; readonly modifyTables: readonly components["schemas"]["Table"][]; }; + /** + * @description Menu of the context, targets by uuid, parents by technical name + * @default [] + */ + readonly menuItems?: readonly { + readonly label: string; + readonly icon?: string; + /** @enum {string} */ + readonly targetType: "view" | "table" | "url"; + readonly targetUuid?: string; + readonly url?: string; + readonly technicalName?: string; + readonly parentTechnicalName?: string; + }[]; + /** + * @description Views without a table that the menu points at + * @default [] + */ + readonly gridViews?: readonly { + readonly [key: string]: Record; + }[]; }; }; }; diff --git a/tests/unit/Model/ContextSchemeTest.php b/tests/unit/Model/ContextSchemeTest.php new file mode 100644 index 0000000000..5da5236f1e --- /dev/null +++ b/tests/unit/Model/ContextSchemeTest.php @@ -0,0 +1,66 @@ + [], 'modifyTables' => []]; + + public function testCreateFromInputCarriesEveryPart(): void { + $scheme = ContextScheme::createFromInputArray([ + 'name' => 'Intake', 'icon' => 'inbox', 'description' => 'desc', + 'nodes' => [['node_type' => 0, 'node_uuid' => 'u1', 'permissions' => 1]], + 'tables' => self::TABLES, + 'menuItems' => [['label' => 'Home', 'targetType' => 'view', 'targetUuid' => 'v1']], + 'gridViews' => [['uuid' => 'v1', 'title' => 'Home']], + ]); + + $this->assertSame('Intake', $scheme->getName()); + $this->assertSame('inbox', $scheme->getIcon()); + $this->assertCount(1, $scheme->getNodes()); + $this->assertCount(1, $scheme->getMenuItems()); + $this->assertCount(1, $scheme->getGridViews()); + $this->assertSame(self::TABLES, $scheme->getTables()); + } + + public function testCreateFromInputWithoutAMenuHasAnEmptyOne(): void { + $scheme = ContextScheme::createFromInputArray(['name' => 'Intake', 'tables' => self::TABLES]); + + $this->assertSame([], $scheme->getMenuItems()); + $this->assertSame([], $scheme->getGridViews()); + } + + public function testCreateFromInputRejectsTablesWithoutBothLists(): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('addTables'); + ContextScheme::createFromInputArray(['name' => 'Intake', 'tables' => ['addTables' => []]]); + } + + public function testCreateFromInputRejectsAMissingName(): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('name'); + ContextScheme::createFromInputArray(['name' => ' ', 'tables' => self::TABLES]); + } + + public function testCreateFromInputRejectsANonListMenu(): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('menuItems'); + ContextScheme::createFromInputArray(['name' => 'Intake', 'tables' => self::TABLES, 'menuItems' => 'home']); + } + + public function testCreateFromInputRejectsAnIconThatIsNotAString(): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('"icon" must be a string'); + ContextScheme::createFromInputArray(['name' => 'Intake', 'tables' => self::TABLES, 'icon' => 7]); + } +} diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index 57499c576f..35a2501451 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -28,6 +28,7 @@ use OCA\Tables\Db\ViewMapper; use OCA\Tables\Errors\PermissionError; use OCA\Tables\Model\MenuItemInput; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\EventDispatcher\IEventDispatcher; use OCP\IDBConnection; use OCP\INavigationManager; @@ -396,6 +397,139 @@ public function testDeleteRemovesTheApplicationBeforeWhatBelongsToIt(): void { $this->assertSame(['application', 'menu'], $calls); } + /** + * The export and the import resolution are private because their only entry points + * need the server container; they are exercised directly here. + */ + private function callPrivate(string $method, array $arguments): mixed { + $reflection = new \ReflectionMethod($this->service, $method); + return $reflection->invokeArgs($this->service, $arguments); + } + + private function gridViewWithWidget(array $content): View { + $gridView = new View(); + $gridView->setId(6); + $gridView->setUuid('view-uuid'); + $gridView->setTitle('Intake home'); + $gridView->setType(View::TYPE_GRID); + $gridView->setGridArray(['widgets' => [['id' => 'w-1', 'type' => 'data', 'content' => $content]], 'layout' => []]); + return $gridView; + } + + private function expectUuidLookups(): void { + $table = new Table(); + $table->setId(4); + $this->tableMapper->method('findByUuid')->willReturnCallback(static function (string $uuid) use ($table): Table { + if ($uuid === 'table-uuid') { + return $table; + } + throw new DoesNotExistException('unknown'); + }); + $view = new View(); + $view->setId(6); + $this->viewMapper->method('findByUuid')->willReturnCallback(static fn (string $uuid): ?View => $uuid === 'view-uuid' ? $view : null); + } + + public function testExportNamesMenuTargetsByUuidAndCarriesTheViewsWithoutATable(): void { + $table = new Table(); + $table->setId(4); + $table->setUuid('table-uuid'); + $this->tableMapper->method('find')->with(4)->willReturn($table); + $this->viewMapper->method('find')->with(6)->willReturn($this->gridViewWithWidget(['target' => ['type' => 'table', 'id' => 4]])); + + $context = $this->contextWithMenu(7, [ + ['label' => 'Intake home', 'icon' => null, 'targetType' => 'view', 'targetId' => 6, 'url' => null, 'technicalName' => 'home'], + ['label' => 'Welcome', 'icon' => null, 'targetType' => 'table', 'targetId' => 4, 'url' => null, 'technicalName' => 'welcome'], + ['label' => 'Docs', 'icon' => null, 'targetType' => 'url', 'targetId' => null, 'url' => 'https://example.org', 'technicalName' => 'docs'], + ]); + + [$menuItems, $gridViews] = $this->callPrivate('exportMenuItems', [$context]); + + $this->assertSame(['view-uuid', 'table-uuid', null], array_column($menuItems, 'targetUuid')); + $this->assertArrayNotHasKey('targetId', $menuItems[0]); + $this->assertCount(1, $gridViews); + $this->assertSame('view-uuid', $gridViews[0]['uuid']); + $this->assertSame(['type' => 'table', 'id' => 4, 'uuid' => 'table-uuid'], $gridViews[0]['grid']['widgets'][0]['content']['target']); + } + + public function testExportSkipsMenuItemsWhoseTargetIsGone(): void { + $this->tableMapper->method('find')->willThrowException(new DoesNotExistException('gone')); + $context = $this->contextWithMenu(7, [ + ['label' => 'Gone', 'icon' => null, 'targetType' => 'table', 'targetId' => 99, 'url' => null, 'technicalName' => 'gone'], + ['label' => 'Docs', 'icon' => null, 'targetType' => 'url', 'targetId' => null, 'url' => 'https://example.org', 'technicalName' => 'docs'], + ]); + + [$menuItems] = $this->callPrivate('exportMenuItems', [$context]); + + $this->assertSame(['Docs'], array_column($menuItems, 'label')); + } + + public function testExportNamesTheParentOfANestedItemByItsTechnicalName(): void { + $context = $this->contextWithMenu(7, [ + ['id' => 1, 'label' => 'Docs', 'icon' => null, 'targetType' => 'url', 'targetId' => null, 'url' => 'https://example.org', 'technicalName' => 'docs', 'parentId' => null], + ['id' => 2, 'label' => 'API', 'icon' => null, 'targetType' => 'url', 'targetId' => null, 'url' => 'https://example.org/api', 'technicalName' => 'api', 'parentId' => 1], + ]); + + [$menuItems] = $this->callPrivate('exportMenuItems', [$context]); + + $this->assertSame([null, 'docs'], array_column($menuItems, 'parentTechnicalName')); + } + + public function testImportMapsMenuTargetsToLocalIdsAndSkipsWhatItCannotUse(): void { + $this->expectUuidLookups(); + + /** @var list $resolved */ + $resolved = $this->callPrivate('resolveMenuItems', [[ + ['label' => 'Intake home', 'targetType' => 'view', 'targetUuid' => 'view-uuid', 'technicalName' => 'home'], + ['label' => 'Welcome', 'targetType' => 'table', 'targetUuid' => 'table-uuid'], + ['label' => 'Missing', 'targetType' => 'table', 'targetUuid' => 'nope'], + ['label' => 'Weird', 'targetType' => 'kanban', 'targetUuid' => 'x'], + ['label' => ' ', 'targetType' => 'url', 'url' => 'https://example.org'], + ['label' => 'Script', 'targetType' => 'url', 'url' => 'javascript:alert(1)'], + ['label' => 'Docs', 'targetType' => 'url', 'url' => 'https://example.org'], + ]]); + + $this->assertSame(['Intake home', 'Welcome', 'Docs'], array_map(static fn (MenuItemInput $item) => $item->getLabel(), $resolved)); + $this->assertSame([6, 4, null], array_map(static fn (MenuItemInput $item) => $item->getTargetId(), $resolved)); + } + + public function testImportPutsAChildUnderItsParentWhenTheParentSurvivesAndIsTopLevel(): void { + $this->expectUuidLookups(); + + /** @var list $resolved */ + $resolved = $this->callPrivate('resolveMenuItems', [[ + ['label' => 'Missing', 'targetType' => 'table', 'targetUuid' => 'nope', 'technicalName' => 'missing'], + ['label' => 'Docs', 'url' => 'https://example.org', 'technicalName' => 'docs'], + ['label' => 'API', 'url' => 'https://example.org/api', 'technicalName' => 'api', 'parentTechnicalName' => 'docs'], + ['label' => 'Orphan', 'url' => 'https://example.org/o', 'parentTechnicalName' => 'missing'], + ['label' => 'Deep', 'url' => 'https://example.org/d', 'parentTechnicalName' => 'api'], + ]]); + + $this->assertSame(['Docs', 'API', 'Orphan', 'Deep'], array_map(static fn (MenuItemInput $item) => $item->getLabel(), $resolved)); + $this->assertSame([null, 0, null, null], array_map(static fn (MenuItemInput $item) => $item->getParentIndex(), $resolved)); + } + + public function testImportDropsGridWidgetsWhoseTargetIsNotHere(): void { + $this->expectUuidLookups(); + + $grid = $this->callPrivate('resolveGridTargets', [[ + 'widgets' => [ + ['id' => 'w-1', 'type' => 'data', 'content' => ['target' => ['type' => 'table', 'id' => 40, 'uuid' => 'table-uuid']]], + ['id' => 'w-2', 'type' => 'data', 'content' => ['target' => ['type' => 'view', 'id' => 60, 'uuid' => 'elsewhere']]], + ['id' => 'w-3', 'type' => 'text', 'content' => ['text' => 'hi']], + ], + 'layout' => [ + ['widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 12, 'gridHeight' => 4], + ['widgetId' => 'w-2', 'gridX' => 0, 'gridY' => 4, 'gridWidth' => 12, 'gridHeight' => 4], + ['widgetId' => 'w-3', 'gridX' => 0, 'gridY' => 8, 'gridWidth' => 12, 'gridHeight' => 2], + ], + ]]); + + $this->assertSame(['w-1', 'w-3'], array_column($grid['widgets'], 'id')); + $this->assertSame(['type' => 'table', 'id' => 4], $grid['widgets'][0]['content']['target']); + $this->assertSame(['w-1', 'w-3'], array_column($grid['layout'], 'widgetId')); + } + public function testUpdateReordersStartPageContentsFromSubmittedNodes(): void { $context = new Context(); $context->id = 7; From a2a1ac9643c737c604d1f5c0b0bb0013bc9393d7 Mon Sep 17 00:00:00 2001 From: Remko Date: Wed, 14 Oct 2026 15:57:53 +0200 Subject: [PATCH 2/4] fix(contexts): import schemes without losing the menu or failing on what 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 Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/Controller/ContextController.php | 9 +- lib/Model/ContextScheme.php | 42 +++- lib/Service/ContextService.php | 129 ++++++++---- openapi.json | 5 +- src/types/openapi/openapi.ts | 6 +- .../unit/Controller/ContextControllerTest.php | 79 +++++++ tests/unit/Model/ContextSchemeTest.php | 41 +++- tests/unit/Service/ContextServiceTest.php | 197 +++++++++++++++++- 8 files changed, 450 insertions(+), 58 deletions(-) create mode 100644 tests/unit/Controller/ContextControllerTest.php diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index 9d363a87b3..50cfec7846 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -362,7 +362,7 @@ public function previewSchemeChanges(int $contextId, array $updateScheme): DataR * @param string $description Description of the context * @param list $nodes Ordered meta data of the related nodes * @psalm-param array{addTables: list, modifyTables: list} $tables Tables to be added or modified - * @psalm-param list $menuItems Menu of the context, targets by uuid, parents by technical name + * @psalm-param ?list $menuItems Menu of the context, targets by uuid, parents by technical name; leave it out to keep the current menu * @psalm-param list> $gridViews Views without a table that the menu points at * * @return DataResponse, array{}>|DataResponse @@ -374,7 +374,7 @@ public function previewSchemeChanges(int $contextId, array $updateScheme): DataR */ #[NoAdminRequired] #[RequirePermission(permission: Application::PERMISSION_MANAGE, typeParam: 'context', idParam: 'contextId')] - public function importScheme(int $contextId, string $name, string $iconName, string $description, array $nodes, array $tables, array $menuItems = [], array $gridViews = []): DataResponse { + public function importScheme(int $contextId, string $name, string $iconName, string $description, array $nodes, array $tables, ?array $menuItems = null, array $gridViews = []): DataResponse { try { $scheme = ContextScheme::createFromInputArray(['name' => $name, 'icon' => $iconName, 'description' => $description, 'nodes' => $nodes, 'tables' => $tables, 'menuItems' => $menuItems, 'gridViews' => $gridViews]); } catch (InvalidArgumentException $e) { @@ -388,7 +388,10 @@ public function importScheme(int $contextId, string $name, string $iconName, str } catch (\InvalidArgumentException $e) { $this->db->rollBack(); return $this->handleBadRequestError(new BadRequestError($e->getMessage(), $e->getCode(), $e)); - } catch (Exception|MultipleObjectsReturnedException $e) { + } catch (BadRequestError $e) { + $this->db->rollBack(); + return $this->handleBadRequestError($e); + } catch (Exception|MultipleObjectsReturnedException|InternalError $e) { $this->db->rollBack(); return $this->handleError($e); } catch (PermissionError $e) { diff --git a/lib/Model/ContextScheme.php b/lib/Model/ContextScheme.php index 51e29bd141..6c4b288ac6 100644 --- a/lib/Model/ContextScheme.php +++ b/lib/Model/ContextScheme.php @@ -7,6 +7,7 @@ namespace OCA\Tables\Model; +use DateTime; use InvalidArgumentException; use JsonSerializable; @@ -19,7 +20,7 @@ public function __construct( protected ?array $nodes = [], protected ?array $pages = [], protected ?array $tables = [], - protected array $menuItems = [], + protected ?array $menuItems = [], protected array $gridViews = [], ) { } @@ -48,6 +49,7 @@ public static function createFromInputArray(array $data): self { throw new InvalidArgumentException('The scheme property "' . $optional . '" must be a string.'); } } + self::assertGridViewsValid($data['gridViews'] ?? []); return new self( name: $data['name'], icon: $data['icon'] ?? null, @@ -55,11 +57,44 @@ public static function createFromInputArray(array $data): self { nodes: array_values($data['nodes'] ?? []), pages: [], tables: $tables, - menuItems: array_values($data['menuItems'] ?? []), + // A scheme without a menu leaves the application's menu alone; an empty one clears it + menuItems: isset($data['menuItems']) ? array_values($data['menuItems']) : null, gridViews: array_values($data['gridViews'] ?? []), ); } + /** + * Every view without a table in a scheme needs a uuid and a title; its other fields are text or absent, + * and its dates are in the format the export writes. + * + * @throws InvalidArgumentException + */ + private static function assertGridViewsValid(array $gridViews): void { + foreach ($gridViews as $gridView) { + if (!is_array($gridView)) { + throw new InvalidArgumentException('Every entry of "gridViews" must be an object.'); + } + foreach (['uuid', 'title'] as $required) { + if (!is_string($gridView[$required] ?? null) || trim($gridView[$required]) === '') { + throw new InvalidArgumentException('Every entry of "gridViews" needs a "' . $required . '".'); + } + } + foreach (['emoji', 'description', 'createdAt', 'lastEditAt'] as $optional) { + if (isset($gridView[$optional]) && !is_string($gridView[$optional])) { + throw new InvalidArgumentException('The grid view property "' . $optional . '" must be a string.'); + } + } + foreach (['createdAt', 'lastEditAt'] as $date) { + if (isset($gridView[$date]) && DateTime::createFromFormat('Y-m-d H:i:s', $gridView[$date]) === false) { + throw new InvalidArgumentException('The grid view property "' . $date . '" must be a date like 2026-10-14 12:00:00.'); + } + } + if (isset($gridView['grid']) && !is_array($gridView['grid'])) { + throw new InvalidArgumentException('The grid view property "grid" must be an object.'); + } + } + } + public function getName(): ?string { return $this->name ?? ''; } @@ -86,8 +121,9 @@ public function getTables(): ?array { /** * Menu items point at their target by uuid, so a scheme can be imported on another instance. + * Null when the scheme has no menu, which leaves the application's menu as it is. */ - public function getMenuItems(): array { + public function getMenuItems(): ?array { return $this->menuItems; } diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 3d0c7bc7e7..85c79c2486 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -924,10 +924,11 @@ public function importContext(Table $table, array $context, int $tableOldId): ?C */ public function getScheme(int $contextId, string $userId): ContextScheme { $context = $this->contextMapper->findById($contextId); - $tableService = \OCP\Server::get(TableService::class); - $viewService = \OCP\Server::get(ViewService::class); + $tableService = $this->tableService(); + $viewService = $this->viewService(); $nodes = $context->getNodes(); $tables = []; + $nodeGridViews = []; foreach ($nodes as &$node) { if ($node['node_type'] === Application::NODE_TYPE_TABLE) { @@ -946,7 +947,12 @@ public function getScheme(int $contextId, string $userId): ContextScheme { $view = $viewService->find($node['node_id']); $node['node_uuid'] = $view->getUuid(); $node['node_title'] = $view->getTitle(); - if ($view->getTableId() === null || isset($tables[$view->getTableId()])) { + // A view without a table travels with the scheme, also when no menu item opens it + if ($view->isStandalone()) { + $nodeGridViews[$view->getUuid()] = $this->exportGridView($view); + continue; + } + if (isset($tables[$view->getTableId()])) { continue; } $tables[$view->getTableId()] = $tableService->getScheme($view->getTableId(), $userId)->jsonSerialize(); @@ -956,7 +962,8 @@ public function getScheme(int $contextId, string $userId): ContextScheme { } } - [$menuItems, $gridViews] = $this->exportMenuItems($context); + [$menuItems, $menuGridViews] = $this->exportMenuItems($context); + $gridViews = array_values(array_column($menuGridViews, null, 'uuid') + $nodeGridViews); return new ContextScheme( $context->getName(), @@ -1077,11 +1084,12 @@ private function widgetTarget(mixed $content): ?array { /** * Points the data widgets of an imported grid at the local tables and views by uuid. A widget - * whose target does not exist here is dropped together with its place in the layout. + * whose target does not exist here, or whose rows the importer cannot read, is dropped together + * with its place in the layout. * * @return array{widgets: list>, layout: list>} */ - private function resolveGridTargets(mixed $grid): array { + private function resolveGridTargets(mixed $grid, string $userId): array { $widgets = is_array($grid) && is_array($grid['widgets'] ?? null) ? $grid['widgets'] : []; $layout = is_array($grid) && is_array($grid['layout'] ?? null) ? $grid['layout'] : []; $resolvedWidgets = []; @@ -1093,11 +1101,11 @@ private function resolveGridTargets(mixed $grid): array { $target = is_array($widget['content'] ?? null) ? ($widget['content']['target'] ?? null) : null; if (is_array($target)) { $targetId = is_string($target['type'] ?? null) && is_string($target['uuid'] ?? null) - ? $this->resolveTargetId($target['type'], $target['uuid']) + ? $this->resolveTargetId($target['type'], $target['uuid'], $userId, false) : null; if ($targetId === null) { $droppedIds[] = $widget['id'] ?? null; - $this->logger->info('Grid widget skipped on import, its table or view does not exist here', ['widget' => $widget['id'] ?? null]); + $this->logger->info('Grid widget skipped on import, its table or view does not exist here or cannot be read', ['widget' => $widget['id'] ?? null]); continue; } $widget['content']['target'] = ['type' => $target['type'], 'id' => $targetId]; @@ -1112,20 +1120,26 @@ private function resolveGridTargets(mixed $grid): array { } /** - * Creates the views without a table of a scheme that do not exist here yet. + * Brings the views without a table of a scheme in. One that exists here already and that the + * importer can manage is reused; otherwise the view is imported, as a copy with its own uuid + * when its uuid is taken. * * @param list> $gridViews + * @return array the local id of every grid view, by its uuid in the scheme * @throws InternalError|BadRequestError */ - private function importGridViews(array $gridViews, string $userId): void { - $viewService = \OCP\Server::get(ViewService::class); + private function importGridViews(array $gridViews, string $userId): array { + $localIds = []; foreach ($gridViews as $viewData) { - if (!isset($viewData['uuid'], $viewData['title']) || !is_string($viewData['uuid']) || $this->viewMapper->findByUuid($viewData['uuid']) !== null) { + $uuid = $viewData['uuid']; + $existing = $this->viewMapper->findByUuid($uuid); + if ($existing !== null && $this->permissionsService->canManageNodeById(Application::NODE_TYPE_VIEW, $existing->getId(), $userId)) { + $localIds[$uuid] = $existing->getId(); continue; } $now = (new \DateTime())->format('Y-m-d H:i:s'); - $viewService->importView(null, [ - 'uuid' => $viewData['uuid'], + $localIds[$uuid] = $this->viewService()->importView(null, [ + 'uuid' => $uuid, 'title' => $viewData['title'], 'emoji' => $viewData['emoji'] ?? null, 'description' => $viewData['description'] ?? '', @@ -1135,40 +1149,57 @@ private function importGridViews(array $gridViews, string $userId): void { 'columnSettings' => [], 'sort' => [], 'filter' => [], - 'grid' => $this->resolveGridTargets($viewData['grid'] ?? null), - ], $userId); + 'grid' => $this->resolveGridTargets($viewData['grid'] ?? null, $userId), + ], $userId)->getId(); } + return $localIds; } /** - * The local id of a table or view a scheme refers to by uuid, or null when it does not exist here. + * The local id of a table or view a scheme refers to by uuid, or null when it does not exist here + * or the importer may not use it: a menu item needs a target the importer can manage, a widget one + * whose rows the importer can read. + * + * @param array $gridViewIds local ids of the grid views brought in by this import, by uuid */ - private function resolveTargetId(string $targetType, ?string $uuid): ?int { + private function resolveTargetId(string $targetType, ?string $uuid, string $userId, bool $forMenu, array $gridViewIds = []): ?int { if ($uuid === null || $uuid === '') { return null; } + if ($targetType === MenuItem::TARGET_VIEW && isset($gridViewIds[$uuid])) { + return $gridViewIds[$uuid]; + } try { - if ($targetType === MenuItem::TARGET_TABLE) { - return $this->tableMapper->findByUuid($uuid)->getId(); - } - if ($targetType === MenuItem::TARGET_VIEW) { - return $this->viewMapper->findByUuid($uuid)?->getId(); - } + $id = match ($targetType) { + MenuItem::TARGET_TABLE => $this->tableMapper->findByUuid($uuid)->getId(), + MenuItem::TARGET_VIEW => $this->viewMapper->findByUuid($uuid)?->getId(), + default => null, + }; } catch (DoesNotExistException|MultipleObjectsReturnedException|Exception) { return null; } - return null; + if ($id === null) { + return null; + } + $nodeType = $targetType === MenuItem::TARGET_TABLE ? Application::NODE_TYPE_TABLE : Application::NODE_TYPE_VIEW; + $usable = $forMenu + ? $this->permissionsService->canManageNodeById($nodeType, $id, $userId) + : $this->permissionsService->canReadRowsByElementId($id, $targetType, $userId); + return $usable ? $id : null; } /** - * Menu items of a scheme with their uuid targets turned into local ids. Items that are - * invalid, or whose target does not exist here, are skipped. + * Menu items of a scheme with their uuid targets turned into local ids. Items that are invalid, + * or whose target does not exist here or cannot be managed by the importer, are skipped. A + * technical name that an earlier item already uses is dropped, so the item gets a derived one. * * @param list> $menuItems + * @param array $gridViewIds local ids of the grid views brought in by this import, by uuid * @return list */ - private function resolveMenuItems(array $menuItems): array { + private function resolveMenuItems(array $menuItems, string $userId, array $gridViewIds = []): array { $prepared = []; + $usedTechnicalNames = []; foreach ($menuItems as $item) { if (!is_array($item)) { continue; @@ -1176,7 +1207,7 @@ private function resolveMenuItems(array $menuItems): array { $targetType = $item['targetType'] ?? MenuItem::TARGET_URL; if ($targetType !== MenuItem::TARGET_URL) { $item['targetId'] = is_string($targetType) && is_string($item['targetUuid'] ?? null) - ? $this->resolveTargetId($targetType, $item['targetUuid']) + ? $this->resolveTargetId($targetType, $item['targetUuid'], $userId, true, $gridViewIds) : null; } $parentTechnicalName = is_string($item['parentTechnicalName'] ?? null) ? $item['parentTechnicalName'] : null; @@ -1187,6 +1218,12 @@ private function resolveMenuItems(array $menuItems): array { $this->logger->info('Menu item skipped on import: {reason}', ['reason' => $e->getMessage()]); continue; } + $technicalName = $item['technicalName'] ?? null; + if (is_string($technicalName) && isset($usedTechnicalNames[$technicalName])) { + unset($item['technicalName']); + } elseif (is_string($technicalName)) { + $usedTechnicalNames[$technicalName] = true; + } $prepared[] = ['item' => $item, 'parent' => $parentTechnicalName]; } @@ -1293,9 +1330,9 @@ public function importScheme(int $contextId, ContextScheme $scheme, string $user $tables = $scheme->getTables() ?? ['addTables' => [], 'modifyTables' => []]; $nodes = $scheme->getNodes() ?? []; - $tableService = \OCP\Server::get(TableService::class); - $viewService = \OCP\Server::get(ViewService::class); - $columnService = \OCP\Server::get(ColumnService::class); + $tableService = $this->tableService(); + $viewService = $this->viewService(); + $columnService = $this->columnService(); foreach ($tables['addTables'] as $tableScheme) { $tableService->validateTableScheme($tableScheme); @@ -1327,7 +1364,7 @@ public function importScheme(int $contextId, ContextScheme $scheme, string $user } // Grid views first, as nodes and menu items may point at them - $this->importGridViews($scheme->getGridViews(), $userId); + $gridViewIds = $this->importGridViews($scheme->getGridViews(), $userId); // Resolve node ids for the new tables and views $resolvedNodes = []; @@ -1340,18 +1377,36 @@ public function importScheme(int $contextId, ContextScheme $scheme, string $user 'permissions' => $node['permissions'], ]; } elseif ($node['node_type'] === Application::NODE_TYPE_VIEW) { - $view = $this->viewMapper->findByUuid($node['node_uuid']); - if ($view === null) { + $viewId = $gridViewIds[$node['node_uuid']] ?? $this->viewMapper->findByUuid($node['node_uuid'])?->getId(); + if ($viewId === null) { throw new DoesNotExistException('View ' . $node['node_uuid'] . ' does not exist'); } $resolvedNodes[] = [ - 'id' => $view->getId(), + 'id' => $viewId, 'type' => $node['node_type'], 'permissions' => $node['permissions'], ]; } } - return $this->update($contextId, $userId, $scheme->getName(), $scheme->getIcon() ?? '', $scheme->getDescription() ?? '', $resolvedNodes, $this->resolveMenuItems($scheme->getMenuItems())); + // A scheme without a menu leaves the menu of the application as it is + $menuItems = $scheme->getMenuItems() === null ? null : $this->resolveMenuItems($scheme->getMenuItems(), $userId, $gridViewIds); + return $this->update($contextId, $userId, $scheme->getName(), $scheme->getIcon() ?? '', $scheme->getDescription() ?? '', $resolvedNodes, $menuItems); + } + + /** + * The services the scheme export and import use. They come from the server container because they + * depend on this service themselves; a test can replace them here. + */ + protected function tableService(): TableService { + return \OCP\Server::get(TableService::class); + } + + protected function viewService(): ViewService { + return \OCP\Server::get(ViewService::class); + } + + protected function columnService(): ColumnService { + return \OCP\Server::get(ColumnService::class); } } diff --git a/openapi.json b/openapi.json index 9021871df1..1e27397cbc 100644 --- a/openapi.json +++ b/openapi.json @@ -13858,8 +13858,9 @@ }, "menuItems": { "type": "array", - "default": [], - "description": "Menu of the context, targets by uuid, parents by technical name", + "nullable": true, + "default": null, + "description": "Menu of the context, targets by uuid, parents by technical name; leave it out to keep the current menu", "items": { "type": "object", "required": [ diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 01a2d5d330..3a69734f45 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -7680,8 +7680,8 @@ export interface operations { readonly modifyTables: readonly components["schemas"]["Table"][]; }; /** - * @description Menu of the context, targets by uuid, parents by technical name - * @default [] + * @description Menu of the context, targets by uuid, parents by technical name; leave it out to keep the current menu + * @default null */ readonly menuItems?: readonly { readonly label: string; @@ -7692,7 +7692,7 @@ export interface operations { readonly url?: string; readonly technicalName?: string; readonly parentTechnicalName?: string; - }[]; + }[] | null; /** * @description Views without a table that the menu points at * @default [] diff --git a/tests/unit/Controller/ContextControllerTest.php b/tests/unit/Controller/ContextControllerTest.php new file mode 100644 index 0000000000..b30573b26a --- /dev/null +++ b/tests/unit/Controller/ContextControllerTest.php @@ -0,0 +1,79 @@ + [], 'modifyTables' => []]; + + private ContextController $controller; + private MockObject $contextService; + private MockObject $db; + + protected function setUp(): void { + parent::setUp(); + $this->contextService = $this->createMock(ContextService::class); + $this->db = $this->createMock(IDBConnection::class); + $l10n = $this->createMock(IL10N::class); + $l10n->method('t')->willReturnArgument(0); + $this->controller = new ContextController( + $this->createMock(IRequest::class), + $this->createMock(LoggerInterface::class), + $l10n, + 'alice', + $this->contextService, + $this->createMock(TableService::class), + $this->db, + $this->createMock(ColumnService::class), + $this->createMock(ViewService::class), + ); + } + + public function testImportOfAMalformedSchemeIsABadRequestThatNeverReachesTheService(): void { + $this->contextService->expects($this->never())->method('importScheme'); + + $response = $this->controller->importScheme(7, 'Intake', 'star', '', [], ['addTables' => []]); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + } + + public function testImportThatTheServiceRefusesAsABadRequestRollsBackWithA400(): void { + $this->contextService->method('importScheme')->willThrowException(new BadRequestError('Unknown widget type')); + $this->db->expects($this->once())->method('rollBack'); + $this->db->expects($this->never())->method('commit'); + + $response = $this->controller->importScheme(7, 'Intake', 'star', '', [], self::TABLES); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + } + + public function testImportThatFailsInternallyRollsBackWithA500(): void { + $this->contextService->method('importScheme')->willThrowException(new InternalError('insert failed')); + $this->db->expects($this->once())->method('rollBack'); + + $response = $this->controller->importScheme(7, 'Intake', 'star', '', [], self::TABLES); + + $this->assertSame(Http::STATUS_INTERNAL_SERVER_ERROR, $response->getStatus()); + } +} diff --git a/tests/unit/Model/ContextSchemeTest.php b/tests/unit/Model/ContextSchemeTest.php index 5da5236f1e..114170012e 100644 --- a/tests/unit/Model/ContextSchemeTest.php +++ b/tests/unit/Model/ContextSchemeTest.php @@ -33,11 +33,44 @@ public function testCreateFromInputCarriesEveryPart(): void { $this->assertSame(self::TABLES, $scheme->getTables()); } - public function testCreateFromInputWithoutAMenuHasAnEmptyOne(): void { - $scheme = ContextScheme::createFromInputArray(['name' => 'Intake', 'tables' => self::TABLES]); + public function testCreateFromInputKeepsAnAbsentMenuApartFromAnEmptyOne(): void { + $withoutMenu = ContextScheme::createFromInputArray(['name' => 'Intake', 'tables' => self::TABLES]); + $emptyMenu = ContextScheme::createFromInputArray(['name' => 'Intake', 'tables' => self::TABLES, 'menuItems' => []]); - $this->assertSame([], $scheme->getMenuItems()); - $this->assertSame([], $scheme->getGridViews()); + $this->assertNull($withoutMenu->getMenuItems()); + $this->assertSame([], $emptyMenu->getMenuItems()); + $this->assertSame([], $withoutMenu->getGridViews()); + } + + /** + * @dataProvider provideInvalidGridViews + */ + public function testCreateFromInputRejectsAnInvalidGridView(array $gridView, string $message): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage($message); + ContextScheme::createFromInputArray(['name' => 'Intake', 'tables' => self::TABLES, 'gridViews' => [$gridView]]); + } + + /** + * @return array, string}> + */ + public static function provideInvalidGridViews(): array { + return [ + 'no uuid' => [['title' => 'Home'], '"uuid"'], + 'a blank title' => [['uuid' => 'v1', 'title' => ' '], '"title"'], + 'an emoji that is not text' => [['uuid' => 'v1', 'title' => 'Home', 'emoji' => 5], '"emoji" must be a string'], + 'a date in another format' => [['uuid' => 'v1', 'title' => 'Home', 'createdAt' => '14-10-2026'], '"createdAt" must be a date'], + 'a grid that is not an object' => [['uuid' => 'v1', 'title' => 'Home', 'grid' => 'widgets'], '"grid" must be an object'], + ]; + } + + public function testCreateFromInputAcceptsACompleteGridView(): void { + $scheme = ContextScheme::createFromInputArray(['name' => 'Intake', 'tables' => self::TABLES, 'gridViews' => [[ + 'uuid' => 'v1', 'title' => 'Home', 'emoji' => '🏠', 'description' => null, + 'createdAt' => '2026-10-14 12:00:00', 'lastEditAt' => '2026-10-14 12:00:00', 'grid' => ['widgets' => [], 'layout' => []], + ]]]); + + $this->assertCount(1, $scheme->getGridViews()); } public function testCreateFromInputRejectsTablesWithoutBothLists(): void { diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index 35a2501451..962dc3fb6d 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -27,6 +27,7 @@ use OCA\Tables\Db\View; use OCA\Tables\Db\ViewMapper; use OCA\Tables\Errors\PermissionError; +use OCA\Tables\Model\ContextScheme; use OCA\Tables\Model\MenuItemInput; use OCP\AppFramework\Db\DoesNotExistException; use OCP\EventDispatcher\IEventDispatcher; @@ -48,6 +49,8 @@ final class ContextServiceTest extends TestCase { private MockObject $tableMapper; private MockObject $viewMapper; private MockObject $permissionsService; + /** @var list the constructor arguments of the service, to build a partial mock with */ + private array $serviceArguments; protected function setUp(): void { parent::setUp(); @@ -68,7 +71,7 @@ protected function setUp(): void { $navigationManager = $this->createMock(INavigationManager::class); $urlGenerator = $this->createMock(IURLGenerator::class); - $this->service = new ContextService( + $this->serviceArguments = [ $this->contextMapper, $this->contextNodeRelMapper, $this->pageMapper, @@ -85,7 +88,20 @@ protected function setUp(): void { $this->tableMapper, $this->viewMapper, $this->menuItemMapper, - ); + ]; + $this->service = new ContextService(...$this->serviceArguments); + } + + /** + * A service whose given methods are mocked, such as the container lookups of the scheme import. + * + * @param list $methods + */ + private function serviceWith(array $methods): ContextService&MockObject { + return $this->getMockBuilder(ContextService::class) + ->setConstructorArgs($this->serviceArguments) + ->onlyMethods($methods) + ->getMock(); } private function contextWithMenu(int $id, array $menuItems): Context { @@ -416,7 +432,9 @@ private function gridViewWithWidget(array $content): View { return $gridView; } - private function expectUuidLookups(): void { + private function expectUuidLookups(bool $usable = true): void { + $this->permissionsService->method('canManageNodeById')->willReturn($usable); + $this->permissionsService->method('canReadRowsByElementId')->willReturn($usable); $table = new Table(); $table->setId(4); $this->tableMapper->method('findByUuid')->willReturnCallback(static function (string $uuid) use ($table): Table { @@ -487,7 +505,7 @@ public function testImportMapsMenuTargetsToLocalIdsAndSkipsWhatItCannotUse(): vo ['label' => ' ', 'targetType' => 'url', 'url' => 'https://example.org'], ['label' => 'Script', 'targetType' => 'url', 'url' => 'javascript:alert(1)'], ['label' => 'Docs', 'targetType' => 'url', 'url' => 'https://example.org'], - ]]); + ], 'user-1']); $this->assertSame(['Intake home', 'Welcome', 'Docs'], array_map(static fn (MenuItemInput $item) => $item->getLabel(), $resolved)); $this->assertSame([6, 4, null], array_map(static fn (MenuItemInput $item) => $item->getTargetId(), $resolved)); @@ -503,7 +521,7 @@ public function testImportPutsAChildUnderItsParentWhenTheParentSurvivesAndIsTopL ['label' => 'API', 'url' => 'https://example.org/api', 'technicalName' => 'api', 'parentTechnicalName' => 'docs'], ['label' => 'Orphan', 'url' => 'https://example.org/o', 'parentTechnicalName' => 'missing'], ['label' => 'Deep', 'url' => 'https://example.org/d', 'parentTechnicalName' => 'api'], - ]]); + ], 'user-1']); $this->assertSame(['Docs', 'API', 'Orphan', 'Deep'], array_map(static fn (MenuItemInput $item) => $item->getLabel(), $resolved)); $this->assertSame([null, 0, null, null], array_map(static fn (MenuItemInput $item) => $item->getParentIndex(), $resolved)); @@ -523,13 +541,180 @@ public function testImportDropsGridWidgetsWhoseTargetIsNotHere(): void { ['widgetId' => 'w-2', 'gridX' => 0, 'gridY' => 4, 'gridWidth' => 12, 'gridHeight' => 4], ['widgetId' => 'w-3', 'gridX' => 0, 'gridY' => 8, 'gridWidth' => 12, 'gridHeight' => 2], ], - ]]); + ], 'user-1']); $this->assertSame(['w-1', 'w-3'], array_column($grid['widgets'], 'id')); $this->assertSame(['type' => 'table', 'id' => 4], $grid['widgets'][0]['content']['target']); $this->assertSame(['w-1', 'w-3'], array_column($grid['layout'], 'widgetId')); } + public function testImportSkipsAMenuTargetTheImporterCannotManage(): void { + $this->expectUuidLookups(false); + + /** @var list $resolved */ + $resolved = $this->callPrivate('resolveMenuItems', [[ + ['label' => 'Welcome', 'targetType' => 'table', 'targetUuid' => 'table-uuid'], + ['label' => 'Docs', 'targetType' => 'url', 'url' => 'https://example.org'], + ], 'user-1']); + + $this->assertSame(['Docs'], array_map(static fn (MenuItemInput $item) => $item->getLabel(), $resolved)); + } + + public function testImportDropsAGridWidgetWhoseRowsTheImporterCannotRead(): void { + $this->expectUuidLookups(false); + + $grid = $this->callPrivate('resolveGridTargets', [[ + 'widgets' => [ + ['id' => 'w-1', 'type' => 'data', 'content' => ['target' => ['type' => 'table', 'id' => 40, 'uuid' => 'table-uuid']]], + ['id' => 'w-2', 'type' => 'text', 'content' => ['text' => 'hi']], + ], + 'layout' => [ + ['widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 12, 'gridHeight' => 4], + ['widgetId' => 'w-2', 'gridX' => 0, 'gridY' => 4, 'gridWidth' => 12, 'gridHeight' => 2], + ], + ], 'user-1']); + + $this->assertSame(['w-2'], array_column($grid['widgets'], 'id')); + $this->assertSame(['w-2'], array_column($grid['layout'], 'widgetId')); + } + + public function testImportGivesALaterItemWithATakenTechnicalNameADerivedOne(): void { + $this->expectUuidLookups(); + + /** @var list $resolved */ + $resolved = $this->callPrivate('resolveMenuItems', [[ + ['label' => 'Docs', 'url' => 'https://example.org/a', 'technicalName' => 'docs'], + ['label' => 'More docs', 'url' => 'https://example.org/b', 'technicalName' => 'docs'], + ], 'user-1']); + + $this->assertSame(['docs', null], array_map(static fn (MenuItemInput $item) => $item->getTechnicalName(), $resolved)); + } + + public function testExportCarriesAGridViewOnceWhenTwoItemsOpenIt(): void { + $this->viewMapper->method('find')->with(6)->willReturn($this->gridViewWithWidget(['text' => 'hi'])); + $context = $this->contextWithMenu(7, [ + ['label' => 'Home', 'icon' => null, 'targetType' => 'view', 'targetId' => 6, 'url' => null, 'technicalName' => 'home'], + ['label' => 'Home again', 'icon' => null, 'targetType' => 'view', 'targetId' => 6, 'url' => null, 'technicalName' => 'home_again'], + ]); + + [, $gridViews] = $this->callPrivate('exportMenuItems', [$context]); + + $this->assertSame(['view-uuid'], array_column($gridViews, 'uuid')); + } + + public function testExportCarriesAGridViewThatIsOnlyAResource(): void { + $context = $this->contextWithMenu(7, []); + $context->setNodes([3 => ['id' => 3, 'node_id' => 6, 'node_type' => Application::NODE_TYPE_VIEW, 'permissions' => 1]]); + $this->contextMapper->method('findById')->with(7)->willReturn($context); + $viewService = $this->createMock(ViewService::class); + $viewService->method('find')->with(6)->willReturn($this->gridViewWithWidget(['text' => 'hi'])); + $service = $this->serviceWith(['tableService', 'viewService']); + $service->method('tableService')->willReturn($this->createMock(TableService::class)); + $service->method('viewService')->willReturn($viewService); + + $scheme = $service->getScheme(7, 'user-1'); + + $this->assertSame(['view-uuid'], array_column($scheme->getGridViews(), 'uuid')); + } + + public function testImportReusesAGridViewTheImporterCanManage(): void { + $existing = new View(); + $existing->setId(6); + $this->viewMapper->method('findByUuid')->with('view-uuid')->willReturn($existing); + $this->permissionsService->method('canManageNodeById')->with(Application::NODE_TYPE_VIEW, 6, 'user-1')->willReturn(true); + $viewService = $this->createMock(ViewService::class); + $viewService->expects($this->never())->method('importView'); + $service = $this->serviceWith(['viewService']); + $service->method('viewService')->willReturn($viewService); + + $ids = $this->callPrivateOn($service, 'importGridViews', [[['uuid' => 'view-uuid', 'title' => 'Home']], 'user-1']); + + $this->assertSame(['view-uuid' => 6], $ids); + } + + public function testImportCopiesAGridViewTheImporterCannotManage(): void { + $existing = new View(); + $existing->setId(6); + $copy = new View(); + $copy->setId(66); + $this->viewMapper->method('findByUuid')->with('view-uuid')->willReturn($existing); + $this->permissionsService->method('canManageNodeById')->willReturn(false); + $viewService = $this->createMock(ViewService::class); + $viewService->expects($this->once())->method('importView')->with(null, $this->callback(static fn (array $view): bool => $view['uuid'] === 'view-uuid' && $view['type'] === View::TYPE_GRID), 'user-1')->willReturn($copy); + $service = $this->serviceWith(['viewService']); + $service->method('viewService')->willReturn($viewService); + + $ids = $this->callPrivateOn($service, 'importGridViews', [[['uuid' => 'view-uuid', 'title' => 'Home']], 'user-1']); + + $this->assertSame(['view-uuid' => 66], $ids); + } + + /** + * A service for importScheme whose container lookups return mocks; the scheme imports no tables. + * + * @param list $alsoMock further methods to mock + */ + private function importingService(MockObject $viewService, array $alsoMock = []): ContextService&MockObject { + $service = $this->serviceWith(array_merge(['tableService', 'viewService', 'columnService'], $alsoMock)); + $service->method('tableService')->willReturn($this->createMock(TableService::class)); + $service->method('viewService')->willReturn($viewService); + $service->method('columnService')->willReturn($this->createMock(ColumnService::class)); + return $service; + } + + private function scheme(array $extra = []): ContextScheme { + return ContextScheme::createFromInputArray(array_merge([ + 'name' => 'Intake portal', + 'icon' => 'star', + 'tables' => ['addTables' => [], 'modifyTables' => []], + ], $extra)); + } + + public function testImportOfASchemeWithoutAMenuLeavesTheMenuAlone(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->menuItemMapper->expects($this->never())->method('deleteAllByContextId'); + + $this->importingService($this->createMock(ViewService::class))->importScheme(7, $this->scheme(), 'user-1'); + } + + public function testImportOfAnEmptyMenuClearsTheMenu(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->menuItemMapper->expects($this->once())->method('deleteAllByContextId')->with(7); + $this->menuItemMapper->method('findByContextId')->willReturn([]); + + $this->importingService($this->createMock(ViewService::class))->importScheme(7, $this->scheme(['menuItems' => []]), 'user-1'); + } + + public function testImportBringsGridViewsInBeforeTheNodesAndTheMenuThatPointAtThem(): void { + $copy = new View(); + $copy->setId(66); + $this->viewMapper->method('findByUuid')->willReturn(null); + $viewService = $this->createMock(ViewService::class); + $viewService->method('importView')->willReturn($copy); + $service = $this->importingService($viewService, ['update']); + $service->expects($this->once())->method('update')->with( + 7, + 'user-1', + 'Intake portal', + 'star', + '', + [['id' => 66, 'type' => Application::NODE_TYPE_VIEW, 'permissions' => 1]], + $this->callback(static fn (?array $items): bool => $items !== null && count($items) === 1 && $items[0]->getTargetId() === 66), + )->willReturn($this->contextWithMenu(7, [])); + + $service->importScheme(7, $this->scheme([ + 'nodes' => [['node_type' => Application::NODE_TYPE_VIEW, 'node_uuid' => 'view-uuid', 'permissions' => 1]], + 'menuItems' => [['label' => 'Home', 'targetType' => 'view', 'targetUuid' => 'view-uuid']], + 'gridViews' => [['uuid' => 'view-uuid', 'title' => 'Home']], + ]), 'user-1'); + } + + private function callPrivateOn(ContextService $service, string $method, array $arguments): mixed { + return (new \ReflectionMethod(ContextService::class, $method))->invokeArgs($service, $arguments); + } + public function testUpdateReordersStartPageContentsFromSubmittedNodes(): void { $context = new Context(); $context->id = 7; From 835fd4291498230f6ff1af05a77b0372918345a1 Mon Sep 17 00:00:00 2001 From: Remko Date: Wed, 14 Oct 2026 16:14:50 +0200 Subject: [PATCH 3/4] fix(contexts): keep the menu and grid views when a scheme is imported 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 --- playwright/e2e/application.spec.ts | 48 +++++++++++++++++++++- src/modules/modals/ImportContextScheme.vue | 20 ++++++++- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/playwright/e2e/application.spec.ts b/playwright/e2e/application.spec.ts index 414f545864..ae995bdc1b 100644 --- a/playwright/e2e/application.spec.ts +++ b/playwright/e2e/application.spec.ts @@ -4,9 +4,10 @@ */ import type { Page } from '@playwright/test' +import { randomUUID } from 'node:crypto' import { test, expect } from '../support/fixtures' import { ocsRequest, type TestUser } from '../support/api' -import { createContext, loadContext } from '../support/commands' +import { createContext, ensureNavigationOpen, loadContext } from '../support/commands' /** * Creates a grid view and an application whose menu opens it, through the API. @@ -99,4 +100,49 @@ test.describe('Applications', () => { await expect(pages.nth(1)).toContainText('1.1') await expect(pages.nth(1)).toContainText('Handbook') }) + + test('A scheme imported through the dialog brings its menu and the page it opens', async ({ userPage: { page, user } }) => { + const source = await createApplicationWithGridPage(page, user, 'exported application', 'Exported page') + const exportResponse = await ocsRequest(page.request, user, { + method: 'GET', + url: `/ocs/v2.php/apps/tables/api/2/contexts/${source.id}/scheme/export?format=json`, + }) + const scheme = (await exportResponse.json()).ocs.data + // A grid view uuid this instance does not know, as for a scheme exported on another instance + const fileContent = JSON.stringify(scheme).split(scheme.gridViews[0].uuid).join(randomUUID()) + const targetResponse = await ocsRequest(page.request, user, { + method: 'POST', + url: '/ocs/v2.php/apps/tables/api/2/contexts?format=json', + data: { name: 'importing application', iconName: 'star' }, + }) + const target = (await targetResponse.json()).ocs.data + + await page.goto('/index.php/apps/tables') + await ensureNavigationOpen(page) + 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 }) + + const dialog = page.getByRole('dialog', { name: /application scheme/ }) + await dialog.locator('input[type=file]').setInputFiles({ + name: 'exported-application.json', + mimeType: 'application/json', + buffer: Buffer.from(fileContent), + }) + await dialog.getByRole('button', { name: 'Preview' }).click() + 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 + + expect(response.status()).toBe(200) + expect(response.request().postDataJSON().menuItems).toHaveLength(1) + await expect(dialog).toContainText('Import completed successfully.') + const importedResponse = await ocsRequest(page.request, user, { + method: 'GET', + url: `/ocs/v2.php/apps/tables/api/2/contexts/${target.id}?format=json`, + }) + const imported = (await importedResponse.json()).ocs.data + expect(imported.menuItems.map((item: { label: string }) => item.label)).toEqual(['Exported page']) + }) }) diff --git a/src/modules/modals/ImportContextScheme.vue b/src/modules/modals/ImportContextScheme.vue index 57b695b996..f0ed4e8d3f 100644 --- a/src/modules/modals/ImportContextScheme.vue +++ b/src/modules/modals/ImportContextScheme.vue @@ -145,6 +145,8 @@ export default { importFinished: false, preview: null, schemeData: null, + // The scheme as read from the file; the preview rebuilds schemeData without its menu and grid views + uploadedScheme: null, waitForReload: false, mimeTypes: [ 'application/json', @@ -241,8 +243,21 @@ export default { this.loading = false } }, + /** + * Takes the scheme as chosen in the preview, with the menu and grid views of the file. A file + * without a menu leaves menuItems out, so the import keeps the current menu. + * + * @param {object} newScheme the scheme the preview built + */ onUpdateScheme(newScheme) { - this.schemeData = newScheme + const scheme = { ...newScheme } + if (Array.isArray(this.uploadedScheme?.menuItems)) { + scheme.menuItems = this.uploadedScheme.menuItems + } + if (Array.isArray(this.uploadedScheme?.gridViews)) { + scheme.gridViews = this.uploadedScheme.gridViews + } + this.schemeData = scheme }, async actionImport() { if (!this.schemeData) { @@ -300,7 +315,8 @@ export default { reader.readAsText(this.selectedUploadFile, 'UTF-8') reader.onload = (evt) => { try { - this.schemeData = JSON.parse(evt.target.result) + this.uploadedScheme = JSON.parse(evt.target.result) + this.schemeData = this.uploadedScheme } catch (parseError) { this.importFailed = true this.errorMessage = t('tables', 'The selected file is not valid JSON.') From f8ade38473e5a02c70379e985798150aa64abc4f Mon Sep 17 00:00:00 2001 From: Remko Date: Thu, 15 Oct 2026 09:10:55 +0200 Subject: [PATCH 4/4] fix(contexts): let the import keep the menu, reuse its copies and roll 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 --- lib/Controller/ContextController.php | 3 + lib/Service/ContextService.php | 26 +++- playwright/e2e/application.spec.ts | 142 +++++++++++++----- src/modules/modals/ImportContextScheme.vue | 36 +++-- .../modals/ImportContextSchemePreview.vue | 47 ++++++ .../unit/Controller/ContextControllerTest.php | 28 ++++ tests/unit/Service/ContextServiceTest.php | 61 ++++++-- 7 files changed, 277 insertions(+), 66 deletions(-) diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index 50cfec7846..ba34a96d3f 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -400,6 +400,9 @@ public function importScheme(int $contextId, string $name, string $iconName, str } catch (DoesNotExistException $e) { $this->db->rollBack(); return $this->handleNotFoundError(new NotFoundError($e->getMessage(), $e->getCode(), $e)); + } catch (NotFoundError $e) { + $this->db->rollBack(); + return $this->handleNotFoundError($e); } } diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 85c79c2486..8c9ac88c3e 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -34,6 +34,7 @@ use OCA\Tables\Model\MenuItemInput; use OCA\Tables\Model\SortRuleSet; use OCA\Tables\Model\ViewSettings; +use OCA\Tables\Vendor\Symfony\Component\Uid\Uuid; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\AppFramework\Db\TTransactional; @@ -1121,25 +1122,32 @@ private function resolveGridTargets(mixed $grid, string $userId): array { /** * Brings the views without a table of a scheme in. One that exists here already and that the - * importer can manage is reused; otherwise the view is imported, as a copy with its own uuid - * when its uuid is taken. + * importer can manage is reused; otherwise the view is imported. When its uuid belongs to a view + * the importer cannot manage, the view is imported as a copy whose uuid is derived from that uuid + * and the application, so importing the scheme into the application again reuses that copy + * instead of leaving the previous one behind. * * @param list> $gridViews * @return array the local id of every grid view, by its uuid in the scheme - * @throws InternalError|BadRequestError + * @throws InternalError|BadRequestError|Exception|MultipleObjectsReturnedException */ - private function importGridViews(array $gridViews, string $userId): array { + private function importGridViews(array $gridViews, int $contextId, string $userId): array { $localIds = []; foreach ($gridViews as $viewData) { $uuid = $viewData['uuid']; + $importUuid = $uuid; $existing = $this->viewMapper->findByUuid($uuid); - if ($existing !== null && $this->permissionsService->canManageNodeById(Application::NODE_TYPE_VIEW, $existing->getId(), $userId)) { + 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); + } + if ($existing !== null && $this->canManageView($existing, $userId)) { $localIds[$uuid] = $existing->getId(); continue; } $now = (new \DateTime())->format('Y-m-d H:i:s'); $localIds[$uuid] = $this->viewService()->importView(null, [ - 'uuid' => $uuid, + 'uuid' => $importUuid, 'title' => $viewData['title'], 'emoji' => $viewData['emoji'] ?? null, 'description' => $viewData['description'] ?? '', @@ -1155,6 +1163,10 @@ private function importGridViews(array $gridViews, string $userId): array { return $localIds; } + private function canManageView(View $view, string $userId): bool { + return $this->permissionsService->canManageNodeById(Application::NODE_TYPE_VIEW, $view->getId(), $userId); + } + /** * The local id of a table or view a scheme refers to by uuid, or null when it does not exist here * or the importer may not use it: a menu item needs a target the importer can manage, a widget one @@ -1364,7 +1376,7 @@ public function importScheme(int $contextId, ContextScheme $scheme, string $user } // Grid views first, as nodes and menu items may point at them - $gridViewIds = $this->importGridViews($scheme->getGridViews(), $userId); + $gridViewIds = $this->importGridViews($scheme->getGridViews(), $contextId, $userId); // Resolve node ids for the new tables and views $resolvedNodes = []; diff --git a/playwright/e2e/application.spec.ts b/playwright/e2e/application.spec.ts index ae995bdc1b..5da8fcf73a 100644 --- a/playwright/e2e/application.spec.ts +++ b/playwright/e2e/application.spec.ts @@ -102,47 +102,113 @@ test.describe('Applications', () => { }) test('A scheme imported through the dialog brings its menu and the page it opens', async ({ userPage: { page, user } }) => { - const source = await createApplicationWithGridPage(page, user, 'exported application', 'Exported page') - const exportResponse = await ocsRequest(page.request, user, { - method: 'GET', - url: `/ocs/v2.php/apps/tables/api/2/contexts/${source.id}/scheme/export?format=json`, - }) - const scheme = (await exportResponse.json()).ocs.data - // A grid view uuid this instance does not know, as for a scheme exported on another instance - const fileContent = JSON.stringify(scheme).split(scheme.gridViews[0].uuid).join(randomUUID()) - const targetResponse = await ocsRequest(page.request, user, { - method: 'POST', - url: '/ocs/v2.php/apps/tables/api/2/contexts?format=json', - data: { name: 'importing application', iconName: 'star' }, - }) - const target = (await targetResponse.json()).ocs.data + const fileContent = await exportedSchemeFile(page, user, 'exported application', 'Exported page') + const target = await createApplication(page, user, 'importing application', []) - await page.goto('/index.php/apps/tables') - await ensureNavigationOpen(page) - 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 }) - - const dialog = page.getByRole('dialog', { name: /application scheme/ }) - await dialog.locator('input[type=file]').setInputFiles({ - name: 'exported-application.json', - mimeType: 'application/json', - buffer: Buffer.from(fileContent), - }) - await dialog.getByRole('button', { name: 'Preview' }).click() - 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 + const response = await importThroughDialog(page, 'importing application', fileContent, true) expect(response.status()).toBe(200) expect(response.request().postDataJSON().menuItems).toHaveLength(1) - await expect(dialog).toContainText('Import completed successfully.') - const importedResponse = await ocsRequest(page.request, user, { - method: 'GET', - url: `/ocs/v2.php/apps/tables/api/2/contexts/${target.id}?format=json`, - }) - const imported = (await importedResponse.json()).ocs.data - expect(imported.menuItems.map((item: { label: string }) => item.label)).toEqual(['Exported page']) + expect(await menuLabels(page, user, target.id)).toEqual(['Exported page']) + }) + + test('A scheme imported through the dialog with its menu switched off keeps the current menu', async ({ userPage: { page, user } }) => { + const fileContent = await exportedSchemeFile(page, user, 'exported application', 'Exported page') + const target = await createApplication(page, user, 'importing application', [{ label: 'Handbook', targetType: 'url', url: 'https://docs.nextcloud.com' }]) + + const response = await importThroughDialog(page, 'importing application', fileContent, false) + + expect(response.status()).toBe(200) + expect(response.request().postDataJSON()).not.toHaveProperty('menuItems') + expect(await menuLabels(page, user, target.id)).toEqual(['Handbook']) }) }) + +/** + * Exports an application with a grid page in its menu, as the file an import dialog takes. + * + * @param page the page of the test user + * @param user the test user + * @param name name of the exported application + * @param pageTitle title of its grid page + */ +async function exportedSchemeFile(page: Page, user: TestUser, name: string, pageTitle: string): Promise { + const source = await createApplicationWithGridPage(page, user, name, pageTitle) + const exportResponse = await ocsRequest(page.request, user, { + method: 'GET', + url: `/ocs/v2.php/apps/tables/api/2/contexts/${source.id}/scheme/export?format=json`, + }) + const scheme = (await exportResponse.json()).ocs.data + // A grid view uuid this instance does not know, as for a scheme exported on another instance + return JSON.stringify(scheme).split(scheme.gridViews[0].uuid).join(randomUUID()) +} + +/** + * Creates an application with the given menu through the API. + * + * @param page the page of the test user + * @param user the test user + * @param name name of the application + * @param menuItems its menu + */ +async function createApplication(page: Page, user: TestUser, name: string, menuItems: object[]) { + const response = await ocsRequest(page.request, user, { + method: 'POST', + url: '/ocs/v2.php/apps/tables/api/2/contexts?format=json', + data: { name, iconName: 'star', menuItems }, + }) + return (await response.json()).ocs.data +} + +/** + * Imports a scheme file into an application through its "Import scheme" dialog. + * + * @param page the page of the test user + * @param applicationName name of the application to import into + * @param fileContent the scheme file + * @param withMenu whether the Menu switch of the preview stays on + */ +async function importThroughDialog(page: Page, applicationName: string, fileContent: string, withMenu: boolean) { + await page.goto('/index.php/apps/tables') + await ensureNavigationOpen(page) + const contextItem = page.locator('[data-cy="navigationContextItem"]').filter({ hasText: applicationName }).first() + await contextItem.hover() + await contextItem.getByRole('button', { name: /Actions|Open menu/i }).first().click({ force: true }) + // A forced click can land before the opening menu has settled and then misses the item + await page.getByRole('menuitem', { name: 'Import scheme' }).click() + + const dialog = page.getByRole('dialog', { name: /application scheme/ }) + await expect(dialog).toBeVisible() + await dialog.locator('input[type=file]').setInputFiles({ + name: 'exported-application.json', + mimeType: 'application/json', + buffer: Buffer.from(fileContent), + }) + await dialog.getByRole('button', { name: 'Preview' }).click() + const menuSection = dialog.locator('[data-cy="importSchemeMenu"]') + await expect(menuSection).toBeVisible() + if (!withMenu) { + await menuSection.getByText('Menu', { exact: true }).click() + } + 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.') + return response +} + +/** + * The labels of an application's menu, as the API returns it. + * + * @param page the page of the test user + * @param user the test user + * @param contextId id of the application + */ +async function menuLabels(page: Page, user: TestUser, contextId: number): Promise { + const response = await ocsRequest(page.request, user, { + method: 'GET', + url: `/ocs/v2.php/apps/tables/api/2/contexts/${contextId}?format=json`, + }) + const context = (await response.json()).ocs.data + return context.menuItems.map((item: { label: string }) => item.label) +} diff --git a/src/modules/modals/ImportContextScheme.vue b/src/modules/modals/ImportContextScheme.vue index f0ed4e8d3f..5050f05f3a 100644 --- a/src/modules/modals/ImportContextScheme.vue +++ b/src/modules/modals/ImportContextScheme.vue @@ -41,7 +41,11 @@
- +
@@ -104,6 +108,7 @@ import { useTablesStore } from '../../store/store.js' import { mapState, mapActions } from 'pinia' import NcIconTimerSand from '../../shared/components/ncIconTimerSand/NcIconTimerSand.vue' import ImportContextSchemePreview from './ImportContextSchemePreview.vue' +import { NODE_TYPE_VIEW } from '../../shared/constants.ts' import { translate as t } from '@nextcloud/l10n' import { useDataStore } from '../../store/data.js' @@ -244,20 +249,31 @@ export default { } }, /** - * Takes the scheme as chosen in the preview, with the menu and grid views of the file. A file - * without a menu leaves menuItems out, so the import keeps the current menu. + * Takes the scheme as chosen in the preview, with the grid views of the file that its menu or + * its nodes point at; the preview only brings the menu along when the person chose to. * * @param {object} newScheme the scheme the preview built */ onUpdateScheme(newScheme) { - const scheme = { ...newScheme } - if (Array.isArray(this.uploadedScheme?.menuItems)) { - scheme.menuItems = this.uploadedScheme.menuItems - } - if (Array.isArray(this.uploadedScheme?.gridViews)) { - scheme.gridViews = this.uploadedScheme.gridViews + const gridViews = this.usedGridViews(newScheme) + this.schemeData = gridViews.length > 0 ? { ...newScheme, gridViews } : newScheme + }, + /** + * The grid views of the uploaded file that a menu item or a node of the given scheme opens. + * + * @param {object} scheme the scheme the preview built + * @return {Array} + */ + usedGridViews(scheme) { + const gridViews = this.uploadedScheme?.gridViews + if (!Array.isArray(gridViews)) { + return [] } - this.schemeData = scheme + const usedUuids = new Set([ + ...(scheme.menuItems ?? []).filter(item => item.targetType === 'view').map(item => item.targetUuid), + ...(scheme.nodes ?? []).filter(node => Number(node.node_type) === NODE_TYPE_VIEW).map(node => node.node_uuid), + ]) + return gridViews.filter(view => usedUuids.has(view.uuid)) }, async actionImport() { if (!this.schemeData) { diff --git a/src/modules/modals/ImportContextSchemePreview.vue b/src/modules/modals/ImportContextSchemePreview.vue index a7ac0f1911..aa2fccdc98 100644 --- a/src/modules/modals/ImportContextSchemePreview.vue +++ b/src/modules/modals/ImportContextSchemePreview.vue @@ -179,6 +179,33 @@ + + +
+
+ + {{ t('tables', 'Menu') }} + +
+
    +
  • + {{ t('tables', 'From') }}: +
    • +
  • + {{ item.label }} +
    • +
+
    +
  • + {{ t('tables', 'To') }}: +
    • +
  • + {{ item.label }} +
    • +
+
+
+
@@ -226,6 +253,16 @@ export default { } }, }, + // The menu of the uploaded scheme; null when the file has none, which keeps the current menu + menuItems: { + type: Array, + default: null, + }, + // The menu the application has now + currentMenuItems: { + type: Array, + default: () => [], + }, }, emits: ['update'], @@ -236,6 +273,7 @@ export default { importIcon: true, importDescription: true, importNodes: true, + importMenu: true, addTablesConfig: [], removeColumnsConfig: [], modifyTablesConfig: [], @@ -273,6 +311,11 @@ export default { this.emitPayload() } }, + importMenu: { + handler() { + this.emitPayload() + }, + }, addTablesConfig: { handler() { this.emitPayload() @@ -333,6 +376,10 @@ export default { }, nodes: this.importNodes ? this.previewData.nodes.to : this.previewData.nodes.from, } + // Left out, the import keeps the current menu + if (this.menuItems && this.importMenu) { + payload.menuItems = this.menuItems + } this.$emit('update', payload) }, diff --git a/tests/unit/Controller/ContextControllerTest.php b/tests/unit/Controller/ContextControllerTest.php index b30573b26a..b7dbfff001 100644 --- a/tests/unit/Controller/ContextControllerTest.php +++ b/tests/unit/Controller/ContextControllerTest.php @@ -10,8 +10,11 @@ namespace OCA\Tables\Tests\Unit\Controller; use OCA\Tables\Controller\ContextController; +use OCA\Tables\Db\Context; use OCA\Tables\Errors\BadRequestError; use OCA\Tables\Errors\InternalError; +use OCA\Tables\Errors\NotFoundError; +use OCA\Tables\Model\ContextScheme; use OCA\Tables\Service\ColumnService; use OCA\Tables\Service\ContextService; use OCA\Tables\Service\TableService; @@ -76,4 +79,29 @@ public function testImportThatFailsInternallyRollsBackWithA500(): void { $this->assertSame(Http::STATUS_INTERNAL_SERVER_ERROR, $response->getStatus()); } + + public function testImportOfATableThatIsGoneRollsBackWithA404(): void { + $this->contextService->method('importScheme')->willThrowException(new NotFoundError('table not found')); + $this->db->expects($this->once())->method('rollBack'); + + $response = $this->controller->importScheme(7, 'Intake', 'star', '', [], self::TABLES); + + $this->assertSame(Http::STATUS_NOT_FOUND, $response->getStatus()); + } + + public function testImportWithoutAMenuPassesNoMenuToTheService(): void { + $this->contextService->expects($this->once())->method('importScheme') + ->with(7, $this->callback(static fn (ContextScheme $scheme): bool => $scheme->getMenuItems() === null), 'alice') + ->willReturn(new Context()); + + $this->controller->importScheme(7, 'Intake', 'star', '', [], self::TABLES); + } + + public function testImportWithAnEmptyMenuPassesTheEmptyMenuToTheService(): void { + $this->contextService->expects($this->once())->method('importScheme') + ->with(7, $this->callback(static fn (ContextScheme $scheme): bool => $scheme->getMenuItems() === []), 'alice') + ->willReturn(new Context()); + + $this->controller->importScheme(7, 'Intake', 'star', '', [], self::TABLES, []); + } } diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index 962dc3fb6d..7eacdd9534 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -29,6 +29,7 @@ use OCA\Tables\Errors\PermissionError; use OCA\Tables\Model\ContextScheme; use OCA\Tables\Model\MenuItemInput; +use OCA\Tables\Vendor\Symfony\Component\Uid\Uuid; use OCP\AppFramework\Db\DoesNotExistException; use OCP\EventDispatcher\IEventDispatcher; use OCP\IDBConnection; @@ -432,9 +433,13 @@ private function gridViewWithWidget(array $content): View { return $gridView; } - private function expectUuidLookups(bool $usable = true): void { - $this->permissionsService->method('canManageNodeById')->willReturn($usable); - $this->permissionsService->method('canReadRowsByElementId')->willReturn($usable); + /** + * @param bool $canManage whether the importer can manage the targets, as a menu target needs + * @param bool|null $canRead whether the importer can read their rows, as a widget target needs; the same as $canManage when left out + */ + private function expectUuidLookups(bool $canManage = true, ?bool $canRead = null): void { + $this->permissionsService->method('canManageNodeById')->willReturn($canManage); + $this->permissionsService->method('canReadRowsByElementId')->willReturn($canRead ?? $canManage); $table = new Table(); $table->setId(4); $this->tableMapper->method('findByUuid')->willReturnCallback(static function (string $uuid) use ($table): Table { @@ -548,8 +553,8 @@ public function testImportDropsGridWidgetsWhoseTargetIsNotHere(): void { $this->assertSame(['w-1', 'w-3'], array_column($grid['layout'], 'widgetId')); } - public function testImportSkipsAMenuTargetTheImporterCannotManage(): void { - $this->expectUuidLookups(false); + public function testImportSkipsAMenuTargetTheImporterCanReadButCannotManage(): void { + $this->expectUuidLookups(canManage: false, canRead: true); /** @var list $resolved */ $resolved = $this->callPrivate('resolveMenuItems', [[ @@ -560,8 +565,8 @@ public function testImportSkipsAMenuTargetTheImporterCannotManage(): void { $this->assertSame(['Docs'], array_map(static fn (MenuItemInput $item) => $item->getLabel(), $resolved)); } - public function testImportDropsAGridWidgetWhoseRowsTheImporterCannotRead(): void { - $this->expectUuidLookups(false); + public function testImportDropsAGridWidgetWhoseRowsTheImporterCannotReadEvenWhenItCanManageThem(): void { + $this->expectUuidLookups(canManage: true, canRead: false); $grid = $this->callPrivate('resolveGridTargets', [[ 'widgets' => [ @@ -627,7 +632,7 @@ public function testImportReusesAGridViewTheImporterCanManage(): void { $service = $this->serviceWith(['viewService']); $service->method('viewService')->willReturn($viewService); - $ids = $this->callPrivateOn($service, 'importGridViews', [[['uuid' => 'view-uuid', 'title' => 'Home']], 'user-1']); + $ids = $this->callPrivateOn($service, 'importGridViews', [[['uuid' => 'view-uuid', 'title' => 'Home']], 7, 'user-1']); $this->assertSame(['view-uuid' => 6], $ids); } @@ -637,18 +642,52 @@ public function testImportCopiesAGridViewTheImporterCannotManage(): void { $existing->setId(6); $copy = new View(); $copy->setId(66); - $this->viewMapper->method('findByUuid')->with('view-uuid')->willReturn($existing); + $this->viewMapper->method('findByUuid')->willReturnCallback(static fn (string $uuid): ?View => $uuid === 'view-uuid' ? $existing : null); $this->permissionsService->method('canManageNodeById')->willReturn(false); + $copyUuid = $this->copyUuid(7, 'view-uuid'); $viewService = $this->createMock(ViewService::class); - $viewService->expects($this->once())->method('importView')->with(null, $this->callback(static fn (array $view): bool => $view['uuid'] === 'view-uuid' && $view['type'] === View::TYPE_GRID), 'user-1')->willReturn($copy); + $viewService->expects($this->once())->method('importView')->with(null, $this->callback(static fn (array $view): bool => $view['uuid'] === $copyUuid && $view['type'] === View::TYPE_GRID), 'user-1')->willReturn($copy); $service = $this->serviceWith(['viewService']); $service->method('viewService')->willReturn($viewService); - $ids = $this->callPrivateOn($service, 'importGridViews', [[['uuid' => 'view-uuid', 'title' => 'Home']], 'user-1']); + $ids = $this->callPrivateOn($service, 'importGridViews', [[['uuid' => 'view-uuid', 'title' => 'Home']], 7, 'user-1']); $this->assertSame(['view-uuid' => 66], $ids); } + public function testImportingAgainIntoTheApplicationReusesItsEarlierCopy(): void { + $existing = new View(); + $existing->setId(6); + $copy = new View(); + $copy->setId(66); + $copyUuid = $this->copyUuid(7, 'view-uuid'); + $this->viewMapper->method('findByUuid')->willReturnCallback(static fn (string $uuid): ?View => match ($uuid) { + 'view-uuid' => $existing, + $copyUuid => $copy, + default => null, + }); + $this->permissionsService->method('canManageNodeById')->willReturnCallback(static fn (int $nodeType, int $id): bool => $id === 66); + $viewService = $this->createMock(ViewService::class); + $viewService->expects($this->never())->method('importView'); + $service = $this->serviceWith(['viewService']); + $service->method('viewService')->willReturn($viewService); + + $ids = $this->callPrivateOn($service, 'importGridViews', [[['uuid' => 'view-uuid', 'title' => 'Home']], 7, 'user-1']); + + $this->assertSame(['view-uuid' => 66], $ids); + } + + public function testACopyIntoAnotherApplicationGetsAnotherUuid(): void { + $this->assertNotSame($this->copyUuid(7, 'view-uuid'), $this->copyUuid(8, 'view-uuid')); + } + + /** + * The uuid importGridViews gives the copy of a grid view the importer cannot manage. + */ + private function copyUuid(int $contextId, string $uuid): string { + return Uuid::v5(Uuid::fromString(Uuid::NAMESPACE_URL), 'tables-context-' . $contextId . '/' . $uuid)->toRfc4122(); + } + /** * A service for importScheme whose container lookups return mocks; the scheme imports no tables. *