From b78bbed8560911b2201d7bc62daf1b3130503eb1 Mon Sep 17 00:00:00 2001 From: Remko Date: Mon, 12 Oct 2026 13:27:48 +0200 Subject: [PATCH 1/6] feat(contexts): save the menu of an application Creating or updating an application (context) can now set its menu. The menu items are validated at the controller boundary, and invalid input is answered with a 400 instead of a 500. - every save replaces the whole menu inside the same transaction as the rest of the update - the views and tables the menu opens become nodes of the application, with read permission, so everyone it is shared with can open them; the owner must be able to manage them - a menu item without a technical name gets one derived from its label, with a numeric suffix when the menu already uses it - only http and https links are accepted Co-authored-by: Ruben van der Linde Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Controller/ContextController.php | 13 +- lib/Model/MenuItemInput.php | 151 ++++++++++++++++ lib/Service/ContextService.php | 175 ++++++++++++++++++- openapi.json | 78 +++++++++ src/types/openapi/openapi.ts | 28 +++ tests/unit/Model/MenuItemInputTest.php | 72 ++++++++ tests/unit/Service/ContextServiceTest.php | 201 +++++++++++++++++++++- 7 files changed, 707 insertions(+), 11 deletions(-) create mode 100644 lib/Model/MenuItemInput.php create mode 100644 tests/unit/Model/MenuItemInputTest.php diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index e87f8ca440..858c1717bb 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\MenuItemInput; use OCA\Tables\ResponseDefinitions; use OCA\Tables\Service\ColumnService; use OCA\Tables\Service\ContextService; @@ -100,6 +101,7 @@ public function show(int $contextId): DataResponse { * @param string $iconName Material design icon name of the context * @param string $description Descriptive text of the context * @psalm-param list $nodes optional nodes to be connected to this context + * @psalm-param ?list $menuItems optional menu of the context, in order; the views and tables it opens become nodes of the context * * @return DataResponse|DataResponse * @@ -108,7 +110,7 @@ public function show(int $contextId): DataResponse { * 403: lacking permissions on a resource */ #[NoAdminRequired] - public function create(string $name, string $iconName, string $description = '', array $nodes = []): DataResponse { + public function create(string $name, string $iconName, string $description = '', array $nodes = [], ?array $menuItems = null): DataResponse { try { if (!$this->isValidIcon($iconName)) { return new DataResponse(['message' => 'Invalid icon name'], Http::STATUS_BAD_REQUEST); @@ -120,13 +122,14 @@ public function create(string $name, string $iconName, string $description = '', $this->sanitizeInputNodes($nodes), $this->userId, 0, + $menuItems !== null ? MenuItemInput::createListFromInputArray($menuItems) : null, )->jsonSerialize()); } catch (Exception $e) { return $this->handleError($e); } catch (PermissionError $e) { return $this->handlePermissionError($e); } catch (InvalidArgumentException $e) { - return $this->handleError(new InternalError($e->getMessage(), $e->getCode(), $e)); + return $this->handleBadRequestError(new BadRequestError($e->getMessage(), $e->getCode(), $e)); } } @@ -138,6 +141,7 @@ public function create(string $name, string $iconName, string $description = '', * @param ?string $iconName provide this parameter to set a new icon * @param ?string $description provide this parameter to set a new description * @param ?array{id: int, type: int, permissions: int, order: int} $nodes provide this parameter to set a new list of nodes. + * @psalm-param ?list $menuItems provide this parameter to replace the menu; the views and tables it opens become nodes of the context * * @return DataResponse|DataResponse * @@ -149,7 +153,7 @@ public function create(string $name, string $iconName, string $description = '', * @CanManageContext */ #[NoAdminRequired] - public function update(int $contextId, ?string $name, ?string $iconName, ?string $description, ?array $nodes): DataResponse { + public function update(int $contextId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $menuItems = null): DataResponse { try { if ($iconName !== null && !$this->isValidIcon($iconName)) { return new DataResponse(['message' => 'Invalid icon name'], Http::STATUS_BAD_REQUEST); @@ -162,6 +166,7 @@ public function update(int $contextId, ?string $name, ?string $iconName, ?string $iconName, $description, $nodes, + $menuItems !== null ? MenuItemInput::createListFromInputArray($menuItems) : null, )->jsonSerialize()); } catch (Exception|MultipleObjectsReturnedException $e) { return $this->handleError($e); @@ -169,6 +174,8 @@ public function update(int $contextId, ?string $name, ?string $iconName, ?string return $this->handlePermissionError($e); } catch (DoesNotExistException $e) { return $this->handleNotFoundError(new NotFoundError($e->getMessage(), $e->getCode(), $e)); + } catch (InvalidArgumentException $e) { + return $this->handleBadRequestError(new BadRequestError($e->getMessage(), $e->getCode(), $e)); } } diff --git a/lib/Model/MenuItemInput.php b/lib/Model/MenuItemInput.php new file mode 100644 index 0000000000..5f58aba545 --- /dev/null +++ b/lib/Model/MenuItemInput.php @@ -0,0 +1,151 @@ +label) === '' || mb_strlen($this->label) > self::MAX_LABEL_LENGTH) { + throw new InvalidArgumentException('A menu item needs a label of at most ' . self::MAX_LABEL_LENGTH . ' characters.'); + } + if ($this->icon !== null && mb_strlen($this->icon) > self::MAX_ICON_LENGTH) { + throw new InvalidArgumentException('A menu item icon name must not exceed ' . self::MAX_ICON_LENGTH . ' characters.'); + } + if (!in_array($this->targetType, MenuItem::TARGET_TYPES, true)) { + throw new InvalidArgumentException('Unexpected menu item target type, expected one of: ' . implode(', ', MenuItem::TARGET_TYPES) . '.'); + } + if ($this->targetType === MenuItem::TARGET_URL) { + self::assertUrlValid($this->url); + } elseif ($this->targetId === null || $this->targetId <= 0) { + throw new InvalidArgumentException('A menu item pointing at a ' . $this->targetType . ' needs its id.'); + } + if ($this->technicalName !== null) { + self::assertTechnicalNameValid($this->technicalName); + } + } + + /** + * @throws InvalidArgumentException + */ + public static function createFromInputArray(mixed $data): self { + if (!is_array($data)) { + throw new InvalidArgumentException('A menu item must be an object.'); + } + if (!is_string($data['label'] ?? null)) { + throw new InvalidArgumentException('A menu item needs a label.'); + } + $targetType = $data['targetType'] ?? MenuItem::TARGET_URL; + if (!is_string($targetType)) { + throw new InvalidArgumentException('The target type of a menu item must be a string.'); + } + $isUrl = $targetType === MenuItem::TARGET_URL; + $targetId = $data['targetId'] ?? null; + if (!$isUrl && !is_int($targetId)) { + throw new InvalidArgumentException('A menu item pointing at a ' . $targetType . ' needs its id.'); + } + foreach (['icon', 'url', 'technicalName'] as $optional) { + if (isset($data[$optional]) && !is_string($data[$optional])) { + throw new InvalidArgumentException('The menu item property "' . $optional . '" must be a string.'); + } + } + + return new self( + label: trim($data['label']), + icon: self::emptyToNull($data['icon'] ?? null), + targetType: $targetType, + targetId: $isUrl ? null : $targetId, + url: $isUrl ? trim((string)($data['url'] ?? '')) : null, + technicalName: self::emptyToNull($data['technicalName'] ?? null), + ); + } + + /** + * @return list + * @throws InvalidArgumentException + */ + public static function createListFromInputArray(mixed $data): array { + if (!is_array($data) || !array_is_list($data)) { + throw new InvalidArgumentException('The menu items must be a list.'); + } + return array_map(static fn (mixed $item): self => self::createFromInputArray($item), $data); + } + + /** + * @throws InvalidArgumentException + */ + public static function assertTechnicalNameValid(string $technicalName): void { + if (strlen($technicalName) > self::MAX_TECHNICAL_NAME_LENGTH || !preg_match(self::TECHNICAL_NAME_PATTERN, $technicalName)) { + throw new InvalidArgumentException('A technical name must start with a lowercase letter and contain only lowercase letters, numbers and underscores.'); + } + } + + /** + * @throws InvalidArgumentException + */ + private static function assertUrlValid(?string $url): void { + $scheme = $url === null ? null : parse_url($url, PHP_URL_SCHEME); + if ($url === null || mb_strlen($url) > self::MAX_URL_LENGTH + || !is_string($scheme) || !in_array(strtolower($scheme), self::URL_SCHEMES, true) + || parse_url($url, PHP_URL_HOST) === null) { + throw new InvalidArgumentException('A menu item link must be an http or https address of at most ' . self::MAX_URL_LENGTH . ' characters.'); + } + } + + private static function emptyToNull(?string $value): ?string { + if ($value === null) { + return null; + } + $value = trim($value); + return $value === '' ? null : $value; + } + + public function getLabel(): string { + return $this->label; + } + + public function getIcon(): ?string { + return $this->icon; + } + + public function getTargetType(): string { + return $this->targetType; + } + + public function getTargetId(): ?int { + return $this->targetId; + } + + public function getUrl(): ?string { + return $this->url; + } + + public function getTechnicalName(): ?string { + return $this->technicalName; + } +} diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 32d0ed0c28..bd0ecfca6f 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -14,6 +14,7 @@ use OCA\Tables\Db\ContextMapper; use OCA\Tables\Db\ContextNodeRelation; use OCA\Tables\Db\ContextNodeRelationMapper; +use OCA\Tables\Db\MenuItem; use OCA\Tables\Db\MenuItemMapper; use OCA\Tables\Db\Page; use OCA\Tables\Db\PageContent; @@ -29,6 +30,7 @@ use OCA\Tables\Model\ColumnSettings; use OCA\Tables\Model\ContextScheme; use OCA\Tables\Model\FilterSet; +use OCA\Tables\Model\MenuItemInput; use OCA\Tables\Model\SortRuleSet; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; @@ -132,9 +134,10 @@ public function findById(int $id, ?string $userId): Context { /** * @psalm-param list $nodes + * @param list|null $menuItems * @throws Exception|PermissionError|InvalidArgumentException */ - public function create(string $name, string $iconName, string $description, array $nodes, string $ownerId, int $ownerType): Context { + public function create(string $name, string $iconName, string $description, array $nodes, string $ownerId, int $ownerType, ?array $menuItems = null): Context { $context = new Context(); $context->setName(trim($name)); $context->setIcon(trim($iconName)); @@ -142,7 +145,7 @@ public function create(string $name, string $iconName, string $description, arra $context->setOwnerId($ownerId); $context->setOwnerType($ownerType); - $this->atomic(function () use ($context, $nodes): void { + $this->atomic(function () use ($context, $nodes, $menuItems, $ownerId): void { $this->contextMapper->insert($context); if (!empty($nodes)) { @@ -150,18 +153,179 @@ public function create(string $name, string $iconName, string $description, arra $this->insertNodesFromArray($context, $nodes); } $this->insertPage($context); + $this->replaceMenu($context, $menuItems ?? [], $ownerId); }, $this->dbc); return $context; } + /** + * Replaces the menu of an application and adds the views and tables it points at + * as nodes, so everyone the application is shared with can open them. + * + * @param list $menuItems + * @throws Exception|PermissionError|InvalidArgumentException + */ + protected function replaceMenu(Context $context, array $menuItems, ?string $userId): void { + $this->addMenuTargetsAsNodes($context, $menuItems, $userId); + + $this->menuItemMapper->deleteAllByContextId($context->getId()); + $technicalNames = $this->menuTechnicalNames($menuItems); + foreach ($menuItems as $position => $item) { + $menuItem = new MenuItem(); + $menuItem->setContextId($context->getId()); + $menuItem->setLabel($item->getLabel()); + $menuItem->setIcon($item->getIcon()); + $menuItem->setTargetType($item->getTargetType()); + $menuItem->setTargetId($item->getTargetId()); + $menuItem->setUrl($item->getUrl()); + $menuItem->setTechnicalName($technicalNames[$position]); + $menuItem->setOrder(($position + 1) * 10); + $this->menuItemMapper->insert($menuItem); + } + + $context->setMenuItems(array_map( + static fn (MenuItem $menuItem): array => $menuItem->jsonSerialize(), + $this->menuItemMapper->findByContextId($context->getId()), + )); + } + + /** + * The technical name of every menu item, by position: the one given, or one derived from + * the label with a numeric suffix when this menu already uses it. + * + * @param list $menuItems + * @return array + * @throws InvalidArgumentException + */ + private function menuTechnicalNames(array $menuItems): array { + $names = []; + $taken = []; + foreach ($menuItems as $position => $item) { + $technicalName = $item->getTechnicalName(); + if ($technicalName === null) { + continue; + } + if (isset($taken[$technicalName])) { + throw new InvalidArgumentException('Two menu items share the technical name "' . $technicalName . '".'); + } + $names[$position] = $technicalName; + $taken[$technicalName] = true; + } + foreach ($menuItems as $position => $item) { + if (isset($names[$position])) { + continue; + } + $base = $this->technicalNameFromLabel($item->getLabel()); + $technicalName = $base; + for ($suffix = 2; isset($taken[$technicalName]); $suffix++) { + $technicalName = substr($base, 0, MenuItemInput::MAX_TECHNICAL_NAME_LENGTH - 10) . '_' . $suffix; + } + $names[$position] = $technicalName; + $taken[$technicalName] = true; + } + return $names; + } + + /** + * Targets that no longer exist are left out; the menu marks them broken. + * + * @param list $menuItems + * @throws Exception|PermissionError + */ + private function addMenuTargetsAsNodes(Context $context, array $menuItems, ?string $userId): void { + $missing = []; + foreach ($menuItems as $item) { + $nodeType = match ($item->getTargetType()) { + MenuItem::TARGET_VIEW => Application::NODE_TYPE_VIEW, + MenuItem::TARGET_TABLE => Application::NODE_TYPE_TABLE, + default => null, + }; + $nodeId = $item->getTargetId(); + if ($nodeType !== null && $nodeId !== null) { + $missing[sprintf('t%di%d', $nodeType, $nodeId)] = [$nodeType, $nodeId]; + } + } + $nodes = $context->getNodes() ?? []; + foreach ($nodes as $node) { + unset($missing[sprintf('t%di%d', $node['node_type'] ?? $node['nodeType'], $node['node_id'] ?? $node['nodeId'])]); + } + if ($missing === []) { + return; + } + + $targetIds = [Application::NODE_TYPE_VIEW => [], Application::NODE_TYPE_TABLE => []]; + foreach ($missing as [$nodeType, $nodeId]) { + $targetIds[$nodeType][] = $nodeId; + } + $existing = [ + Application::NODE_TYPE_VIEW => $this->viewMapper->findMany($targetIds[Application::NODE_TYPE_VIEW]), + Application::NODE_TYPE_TABLE => $this->tableMapper->findMany($targetIds[Application::NODE_TYPE_TABLE]), + ]; + + $pages = $context->getPages() ?? []; + $startPageId = null; + foreach ($pages as $pageId => $page) { + if (($page['page_type'] ?? $page['pageType'] ?? null) === Page::TYPE_STARTPAGE) { + $startPageId = (int)$pageId; + break; + } + } + + foreach ($missing as [$nodeType, $nodeId]) { + if (!isset($existing[$nodeType][$nodeId])) { + continue; + } + if (!$this->permissionsService->canManageNodeById($nodeType, $nodeId, $userId)) { + throw new PermissionError(sprintf('Owner cannot manage node %d (type %d)', $nodeId, $nodeType)); + } + $relation = $this->addNodeToContext($context, $nodeId, $nodeType, Application::PERMISSION_READ); + $nodes[$relation->getId()] = [ + 'id' => $relation->getId(), + 'node_id' => $nodeId, + 'node_type' => $nodeType, + 'permissions' => Application::PERMISSION_READ, + ]; + if ($startPageId !== null) { + $content = $this->addNodeRelToPage($relation, null, $startPageId); + $pages[$startPageId]['content'][$content->getId()] = [ + 'order' => $content->getOrder(), + 'node_rel_id' => $relation->getId(), + ]; + } + } + $context->setNodes($nodes); + $context->setPages($pages); + } + + protected function technicalNameFromLabel(string $label): string { + $name = strtolower(trim(preg_replace('/[^a-z0-9]+/i', '_', $label) ?? '', '_')); + $name = preg_replace('/^[^a-z]+/', '', $name) ?? ''; + return substr($name === '' ? 'item' : $name, 0, MenuItemInput::MAX_TECHNICAL_NAME_LENGTH); + } + /** * @psalm-param list $nodes + * @param list|null $menuItems * @throws Exception * @throws DoesNotExistException - * @throws PermissionError|MultipleObjectsReturnedException + * @throws PermissionError|MultipleObjectsReturnedException|InvalidArgumentException */ - public function update(int $contextId, string $userId, ?string $name, ?string $iconName, ?string $description, ?array $nodes): Context { + public function update(int $contextId, string $userId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $menuItems = null): Context { + return $this->atomic( + fn (): Context => $this->applyUpdate($contextId, $userId, $name, $iconName, $description, $nodes, $menuItems), + $this->dbc, + ); + } + + /** + * @psalm-param list $nodes + * @param list|null $menuItems + * @throws Exception + * @throws DoesNotExistException + * @throws PermissionError|MultipleObjectsReturnedException|InvalidArgumentException + */ + private function applyUpdate(int $contextId, string $userId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $menuItems): Context { $context = $this->contextMapper->findById($contextId, $userId); if ($name !== null) { @@ -305,6 +469,9 @@ public function update(int $contextId, string $userId, ?string $name, ?string $i $context->setNodes($currentNodes); $context->setPages($currentPages); } + if ($menuItems !== null) { + $this->replaceMenu($context, $menuItems, $userId); + } return $context; } diff --git a/openapi.json b/openapi.json index a816a48ef5..46f89c28f2 100644 --- a/openapi.json +++ b/openapi.json @@ -12013,6 +12013,45 @@ } } } + }, + "menuItems": { + "type": "array", + "nullable": true, + "default": null, + "description": "optional menu of the context, in order; the views and tables it opens become nodes of the context", + "items": { + "type": "object", + "required": [ + "label", + "targetType" + ], + "properties": { + "label": { + "type": "string" + }, + "icon": { + "type": "string" + }, + "targetType": { + "type": "string", + "enum": [ + "view", + "table", + "url" + ] + }, + "targetId": { + "type": "integer", + "format": "int64" + }, + "url": { + "type": "string" + }, + "technicalName": { + "type": "string" + } + } + } } } } @@ -12445,6 +12484,45 @@ "format": "int64" } } + }, + "menuItems": { + "type": "array", + "nullable": true, + "default": null, + "description": "provide this parameter to replace the menu; the views and tables it opens become nodes of the context", + "items": { + "type": "object", + "required": [ + "label", + "targetType" + ], + "properties": { + "label": { + "type": "string" + }, + "icon": { + "type": "string" + }, + "targetType": { + "type": "string", + "enum": [ + "view", + "table", + "url" + ] + }, + "targetId": { + "type": "integer", + "format": "int64" + }, + "url": { + "type": "string" + }, + "technicalName": { + "type": "string" + } + } + } } } } diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 9558685b0a..0eeb8a1a25 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -6897,6 +6897,20 @@ export interface operations { /** Format: int64 */ readonly permissions?: number; }[]; + /** + * @description optional menu of the context, in order; the views and tables it opens become nodes of the context + * @default null + */ + readonly menuItems?: readonly { + readonly label: string; + readonly icon?: string; + /** @enum {string} */ + readonly targetType: "view" | "table" | "url"; + /** Format: int64 */ + readonly targetId?: number; + readonly url?: string; + readonly technicalName?: string; + }[] | null; }; }; }; @@ -7087,6 +7101,20 @@ export interface operations { /** Format: int64 */ readonly order: number; } | null; + /** + * @description provide this parameter to replace the menu; the views and tables it opens become nodes of the context + * @default null + */ + readonly menuItems?: readonly { + readonly label: string; + readonly icon?: string; + /** @enum {string} */ + readonly targetType: "view" | "table" | "url"; + /** Format: int64 */ + readonly targetId?: number; + readonly url?: string; + readonly technicalName?: string; + }[] | null; }; }; }; diff --git a/tests/unit/Model/MenuItemInputTest.php b/tests/unit/Model/MenuItemInputTest.php new file mode 100644 index 0000000000..49c9e1cd0f --- /dev/null +++ b/tests/unit/Model/MenuItemInputTest.php @@ -0,0 +1,72 @@ + ' Intake home ', 'targetType' => 'view', 'targetId' => 6, 'technicalName' => 'home', 'url' => 'https://ignored.example'], + ['label' => 'Docs', 'targetType' => 'url', 'url' => 'https://example.org/docs', 'targetId' => 99, 'icon' => 'book'], + ['label' => 'Requests', 'targetType' => 'table', 'targetId' => 4, 'icon' => ''], + ]); + + $this->assertSame(['Intake home', null, 'view', 6, null, 'home'], $this->toArray($items[0])); + $this->assertSame(['Docs', 'book', 'url', null, 'https://example.org/docs', null], $this->toArray($items[1])); + $this->assertSame(['Requests', null, 'table', 4, null, null], $this->toArray($items[2])); + } + + public function testATargetTypeOfUrlIsTheDefault(): void { + $item = MenuItemInput::createFromInputArray(['label' => 'Docs', 'url' => 'http://example.org']); + + $this->assertSame('url', $item->getTargetType()); + } + + public static function invalidItems(): array { + return [ + 'not an object' => ['not an item', 'must be an object'], + 'no label' => [['targetType' => 'url', 'url' => 'https://example.org'], 'needs a label'], + 'blank label' => [['label' => ' ', 'targetType' => 'url', 'url' => 'https://example.org'], 'needs a label'], + 'label too long' => [['label' => str_repeat('x', 201), 'url' => 'https://example.org'], 'needs a label'], + 'unknown target type' => [['label' => 'X', 'targetType' => 'kanban', 'targetId' => 1], 'Unexpected menu item target type'], + 'view without id' => [['label' => 'X', 'targetType' => 'view'], 'needs its id'], + 'id as a string' => [['label' => 'X', 'targetType' => 'table', 'targetId' => '4'], 'needs its id'], + 'id of zero' => [['label' => 'X', 'targetType' => 'table', 'targetId' => 0], 'needs its id'], + 'javascript link' => [['label' => 'X', 'url' => 'javascript:alert(1)'], 'http or https'], + 'data link' => [['label' => 'X', 'url' => 'data:text/html,hi'], 'http or https'], + 'link without host' => [['label' => 'X', 'url' => 'https://'], 'http or https'], + 'empty link' => [['label' => 'X', 'url' => ''], 'http or https'], + 'technical name with a hyphen' => [['label' => 'X', 'url' => 'https://example.org', 'technicalName' => 'my-page'], 'technical name'], + 'icon that is not a string' => [['label' => 'X', 'url' => 'https://example.org', 'icon' => 3], '"icon" must be a string'], + ]; + } + + /** + * @dataProvider invalidItems + */ + public function testRejectsInvalidInput(mixed $input, string $message): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage($message); + MenuItemInput::createFromInputArray($input); + } + + public function testRejectsAMenuThatIsNotAList(): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('must be a list'); + MenuItemInput::createListFromInputArray(['first' => ['label' => 'X', 'url' => 'https://example.org']]); + } + + private function toArray(MenuItemInput $item): array { + return [$item->getLabel(), $item->getIcon(), $item->getTargetType(), $item->getTargetId(), $item->getUrl(), $item->getTechnicalName()]; + } +} diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index 9369e5f9e7..d10d323792 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -9,16 +9,24 @@ namespace OCA\Tables\Service; +use InvalidArgumentException; +use OCA\Tables\AppInfo\Application; use OCA\Tables\Db\Context; use OCA\Tables\Db\ContextMapper; +use OCA\Tables\Db\ContextNodeRelation; use OCA\Tables\Db\ContextNodeRelationMapper; +use OCA\Tables\Db\MenuItem; use OCA\Tables\Db\MenuItemMapper; use OCA\Tables\Db\Page; use OCA\Tables\Db\PageContent; use OCA\Tables\Db\PageContentMapper; 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\PermissionError; +use OCA\Tables\Model\MenuItemInput; use OCP\EventDispatcher\IEventDispatcher; use OCP\IDBConnection; use OCP\INavigationManager; @@ -35,6 +43,9 @@ final class ContextServiceTest extends TestCase { private MockObject $pageMapper; private MockObject $pageContentMapper; private MockObject $menuItemMapper; + private MockObject $tableMapper; + private MockObject $viewMapper; + private MockObject $permissionsService; protected function setUp(): void { parent::setUp(); @@ -44,8 +55,10 @@ protected function setUp(): void { $this->pageMapper = $this->createMock(PageMapper::class); $this->pageContentMapper = $this->createMock(PageContentMapper::class); $this->menuItemMapper = $this->createMock(MenuItemMapper::class); + $this->tableMapper = $this->createMock(TableMapper::class); + $this->viewMapper = $this->createMock(ViewMapper::class); + $this->permissionsService = $this->createMock(PermissionsService::class); $logger = $this->createMock(LoggerInterface::class); - $permissionsService = $this->createMock(PermissionsService::class); $userManager = $this->createMock(IUserManager::class); $eventDispatcher = $this->createMock(IEventDispatcher::class); $dbConnection = $this->createMock(IDBConnection::class); @@ -59,7 +72,7 @@ protected function setUp(): void { $this->pageMapper, $this->pageContentMapper, $logger, - $permissionsService, + $this->permissionsService, $userManager, $eventDispatcher, $dbConnection, @@ -67,12 +80,192 @@ protected function setUp(): void { false, $navigationManager, $urlGenerator, - $this->createMock(TableMapper::class), - $this->createMock(ViewMapper::class), + $this->tableMapper, + $this->viewMapper, $this->menuItemMapper, ); } + private function contextWithMenu(int $id, array $menuItems): Context { + $context = new Context(); + $context->id = $id; + $context->setName('Intake portal'); + $context->setOwnerId('user-1'); + $context->setOwnerType(Application::OWNER_TYPE_USER); + $context->setNodes([]); + $context->setPages([]); + $context->setMenuItems($menuItems); + return $context; + } + + /** + * @param list $inserted collects the stored items + */ + private function expectMenuReplacement(int $contextId, array &$inserted): void { + $this->menuItemMapper->expects($this->once())->method('deleteAllByContextId')->with($contextId); + $this->menuItemMapper->method('insert')->willReturnCallback(static function (MenuItem $item) use (&$inserted): MenuItem { + $item->setId(count($inserted) + 1); + $inserted[] = $item; + return $item; + }); + $this->menuItemMapper->method('findByContextId')->willReturnCallback(static function () use (&$inserted): array { + return $inserted; + }); + } + + private function expectExistingTargets(array $views, array $tables): void { + $this->viewMapper->method('findMany')->willReturnCallback(static fn (array $ids): array => array_intersect_key($views, array_flip($ids))); + $this->tableMapper->method('findMany')->willReturnCallback(static fn (array $ids): array => array_intersect_key($tables, array_flip($ids))); + } + + private function expectNodeInsertion(array &$relations): void { + $this->contextNodeRelMapper->method('insert')->willReturnCallback(static function (ContextNodeRelation $relation) use (&$relations): ContextNodeRelation { + $relation->setId(200 + count($relations)); + $relations[] = $relation; + return $relation; + }); + } + + public function testUpdateReplacesTheMenuInOrderAndDerivesTechnicalNamesFromLabels(): void { + $context = $this->contextWithMenu(7, []); + $this->contextMapper->method('findById')->with(7, 'user-1')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->permissionsService->method('canManageNodeById')->willReturn(true); + $this->expectExistingTargets([6 => new View()], [4 => new Table()]); + $relations = []; + $this->expectNodeInsertion($relations); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $updated = $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Intake home', 'targetType' => 'view', 'targetId' => 6, 'technicalName' => 'home'], + ['label' => 'Welcome table!', 'targetType' => 'table', 'targetId' => 4], + ['label' => 'Docs', 'targetType' => 'url', 'url' => 'https://docs.nextcloud.com'], + ])); + + $this->assertSame([10, 20, 30], array_map(static fn (MenuItem $item) => $item->getOrder(), $inserted)); + $this->assertSame(['home', 'welcome_table', 'docs'], array_map(static fn (MenuItem $item) => $item->getTechnicalName(), $inserted)); + $this->assertSame([6, 4, null], array_map(static fn (MenuItem $item) => $item->getTargetId(), $inserted)); + $this->assertSame([null, null, 'https://docs.nextcloud.com'], array_map(static fn (MenuItem $item) => $item->getUrl(), $inserted)); + $this->assertCount(3, $updated->getMenuItems()); + $this->assertSame('Welcome table!', $updated->getMenuItems()[1]['label']); + } + + public function testMenuTargetsBecomeNodesSoRecipientsCanOpenThem(): void { + $context = $this->contextWithMenu(7, []); + $context->setNodes([101 => ['id' => 101, 'node_id' => 4, 'node_type' => Application::NODE_TYPE_TABLE, 'permissions' => 1]]); + $context->setPages([301 => ['id' => 301, 'page_type' => Page::TYPE_STARTPAGE, 'content' => []]]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->permissionsService->method('canManageNodeById')->willReturn(true); + $this->expectExistingTargets([6 => new View()], [4 => new Table()]); + $relations = []; + $this->expectNodeInsertion($relations); + $this->pageContentMapper->method('findByPageAndNodeRelation')->willReturn(null); + $this->pageContentMapper->method('insert')->willReturnCallback(static function (PageContent $content): PageContent { + $content->setId(501); + return $content; + }); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $updated = $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Home', 'targetType' => 'view', 'targetId' => 6], + ['label' => 'Requests', 'targetType' => 'table', 'targetId' => 4], + ])); + + // the table already is a node, only the view is added, with read permission + $this->assertCount(1, $relations); + $this->assertSame([6, Application::NODE_TYPE_VIEW, Application::PERMISSION_READ], [$relations[0]->getNodeId(), $relations[0]->getNodeType(), $relations[0]->getPermissions()]); + $this->assertSame(['id' => 200, 'node_id' => 6, 'node_type' => Application::NODE_TYPE_VIEW, 'permissions' => Application::PERMISSION_READ], $updated->getNodes()[200]); + $this->assertSame(200, $updated->getPages()[301]['content'][501]['node_rel_id']); + } + + public function testAMenuTargetTheOwnerCannotManageIsRefused(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->expectExistingTargets([], [4 => new Table()]); + $this->permissionsService->method('canManageNodeById')->with(Application::NODE_TYPE_TABLE, 4, 'user-1')->willReturn(false); + $this->menuItemMapper->expects($this->never())->method('deleteAllByContextId'); + + $this->expectException(PermissionError::class); + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Requests', 'targetType' => 'table', 'targetId' => 4], + ])); + } + + public function testAMenuTargetThatNoLongerExistsIsKeptWithoutANode(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->expectExistingTargets([], []); + $this->contextNodeRelMapper->expects($this->never())->method('insert'); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Gone', 'targetType' => 'view', 'targetId' => 99], + ])); + + $this->assertSame([99], array_map(static fn (MenuItem $item) => $item->getTargetId(), $inserted)); + } + + public function testDerivedTechnicalNamesGetASuffixWhenTheMenuAlreadyUsesThem(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Docs', 'url' => 'https://example.org/a'], + ['label' => 'Docs', 'url' => 'https://example.org/b'], + ['label' => 'Other', 'url' => 'https://example.org/c', 'technicalName' => 'docs_2'], + ])); + + $this->assertSame(['docs', 'docs_3', 'docs_2'], array_map(static fn (MenuItem $item) => $item->getTechnicalName(), $inserted)); + } + + public function testTwoMenuItemsWithTheSameTechnicalNameAreRefused(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('share the technical name "home"'); + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'A', 'url' => 'https://example.org/a', 'technicalName' => 'home'], + ['label' => 'B', 'url' => 'https://example.org/b', 'technicalName' => 'home'], + ])); + } + + public function testUpdateLeavesTheMenuAloneWhenNoneIsGiven(): void { + $context = $this->contextWithMenu(7, [['id' => 1, 'label' => 'Keep me']]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->menuItemMapper->expects($this->never())->method('deleteAllByContextId'); + + $updated = $this->service->update(7, 'user-1', 'Renamed', null, null, null); + + $this->assertSame('Renamed', $updated->getName()); + $this->assertSame('Keep me', $updated->getMenuItems()[0]['label']); + } + + public function testCreateStoresTheMenuItems(): void { + $this->contextMapper->method('insert')->willReturnCallback(static function (Context $context): Context { + $context->setId(8); + return $context; + }); + $this->pageMapper->method('insert')->willReturnArgument(0); + $inserted = []; + $this->expectMenuReplacement(8, $inserted); + + $created = $this->service->create('Intake portal', 'heart', ' desc ', [], 'user-1', 0, MenuItemInput::createListFromInputArray([ + ['label' => 'Docs', 'targetType' => 'url', 'url' => 'https://example.org'], + ])); + + $this->assertSame('desc', $created->getDescription()); + $this->assertCount(1, $created->getMenuItems()); + $this->assertSame('docs', $inserted[0]->getTechnicalName()); + } + public function testDeleteRemovesTheMenuOfTheApplication(): void { $context = new Context(); $context->id = 7; From c304fd4c2166051f1970d65f65cc9243305060c8 Mon Sep 17 00:00:00 2001 From: Remko Date: Tue, 13 Oct 2026 11:14:29 +0200 Subject: [PATCH 2/6] fix(contexts): answer unusable menu targets alike and keep stored technical names - a new menu target that does not exist gets the same refusal as one the owner cannot manage, so a save reveals nothing about others' ids; a target that was already in the menu is kept and marked broken - an item sent without a technical name keeps the name stored for the same target, so renaming or reordering no longer changes it - removing a menu item keeps its view or table as a resource, as documented - the start page mock gets an id, which PHP 8.5 needs for the array key - a javascript link with a host is tested, which only the scheme check stops - the menu list is mapped with a first-class callable Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Controller/ContextController.php | 2 +- lib/Model/MenuItemInput.php | 2 +- lib/Service/ContextService.php | 104 ++++++++++++++++++---- openapi.json | 2 +- src/types/openapi/openapi.ts | 2 +- tests/unit/Model/MenuItemInputTest.php | 2 + tests/unit/Service/ContextServiceTest.php | 53 +++++++++-- 7 files changed, 138 insertions(+), 29 deletions(-) diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index 858c1717bb..ac236b95ae 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -141,7 +141,7 @@ public function create(string $name, string $iconName, string $description = '', * @param ?string $iconName provide this parameter to set a new icon * @param ?string $description provide this parameter to set a new description * @param ?array{id: int, type: int, permissions: int, order: int} $nodes provide this parameter to set a new list of nodes. - * @psalm-param ?list $menuItems provide this parameter to replace the menu; the views and tables it opens become nodes of the context + * @psalm-param ?list $menuItems provide this parameter to replace the menu; the views and tables it opens become nodes of the context, and an item without a technicalName keeps the one stored for the same target * * @return DataResponse|DataResponse * diff --git a/lib/Model/MenuItemInput.php b/lib/Model/MenuItemInput.php index 5f58aba545..e8e80906f2 100644 --- a/lib/Model/MenuItemInput.php +++ b/lib/Model/MenuItemInput.php @@ -93,7 +93,7 @@ public static function createListFromInputArray(mixed $data): array { if (!is_array($data) || !array_is_list($data)) { throw new InvalidArgumentException('The menu items must be a list.'); } - return array_map(static fn (mixed $item): self => self::createFromInputArray($item), $data); + return array_map(self::createFromInputArray(...), $data); } /** diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index bd0ecfca6f..6fe5553c19 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -161,16 +161,19 @@ public function create(string $name, string $iconName, string $description, arra /** * Replaces the menu of an application and adds the views and tables it points at - * as nodes, so everyone the application is shared with can open them. + * as nodes, so everyone the application is shared with can open them. Removing an + * item leaves its view or table in the application: it stays a resource until it is + * removed as one. * * @param list $menuItems * @throws Exception|PermissionError|InvalidArgumentException */ protected function replaceMenu(Context $context, array $menuItems, ?string $userId): void { - $this->addMenuTargetsAsNodes($context, $menuItems, $userId); + $storedMenu = array_values(array_filter($context->getMenuItems() ?? [], is_array(...))); + $this->addMenuTargetsAsNodes($context, $menuItems, $storedMenu, $userId); $this->menuItemMapper->deleteAllByContextId($context->getId()); - $technicalNames = $this->menuTechnicalNames($menuItems); + $technicalNames = $this->menuTechnicalNames($menuItems, $storedMenu); foreach ($menuItems as $position => $item) { $menuItem = new MenuItem(); $menuItem->setContextId($context->getId()); @@ -191,14 +194,16 @@ protected function replaceMenu(Context $context, array $menuItems, ?string $user } /** - * The technical name of every menu item, by position: the one given, or one derived from - * the label with a numeric suffix when this menu already uses it. + * The technical name of every menu item, by position: the one given; else the stored name of + * an item with the same target, so a rename or a reorder keeps it; else one derived from the + * label, with a numeric suffix when this menu already uses it. * * @param list $menuItems + * @param list> $storedMenu * @return array * @throws InvalidArgumentException */ - private function menuTechnicalNames(array $menuItems): array { + private function menuTechnicalNames(array $menuItems, array $storedMenu): array { $names = []; $taken = []; foreach ($menuItems as $position => $item) { @@ -212,6 +217,19 @@ private function menuTechnicalNames(array $menuItems): array { $names[$position] = $technicalName; $taken[$technicalName] = true; } + $storedNames = $this->storedTechnicalNamesByTarget($storedMenu); + foreach ($menuItems as $position => $item) { + if (isset($names[$position])) { + continue; + } + foreach ($storedNames[self::menuTargetIdentity($item->getTargetType(), $item->getTargetId(), $item->getUrl())] ?? [] as $storedName) { + if (!isset($taken[$storedName])) { + $names[$position] = $storedName; + $taken[$storedName] = true; + break; + } + } + } foreach ($menuItems as $position => $item) { if (isset($names[$position])) { continue; @@ -228,22 +246,69 @@ private function menuTechnicalNames(array $menuItems): array { } /** - * Targets that no longer exist are left out; the menu marks them broken. + * The technical names of the stored menu, grouped by target and in menu order. + * + * @param list> $storedMenu + * @return array> + */ + private function storedTechnicalNamesByTarget(array $storedMenu): array { + $names = []; + foreach ($storedMenu as $storedItem) { + $targetType = $storedItem['targetType'] ?? null; + $technicalName = $storedItem['technicalName'] ?? null; + if (!is_string($targetType) || !is_string($technicalName) || $technicalName === '') { + continue; + } + $targetId = is_int($storedItem['targetId'] ?? null) ? $storedItem['targetId'] : null; + $url = is_string($storedItem['url'] ?? null) ? $storedItem['url'] : null; + $names[self::menuTargetIdentity($targetType, $targetId, $url)][] = $technicalName; + } + return $names; + } + + private static function menuTargetIdentity(string $targetType, ?int $targetId, ?string $url): string { + return $targetType . ':' . ($targetType === MenuItem::TARGET_URL ? (string)$url : (string)$targetId); + } + + /** + * The node a menu target is stored as, keyed so nodes and targets can be compared. + * + * @return array{string, array{int, int}}|null + */ + private static function menuTargetNode(mixed $targetType, mixed $targetId): ?array { + $nodeType = match ($targetType) { + MenuItem::TARGET_VIEW => Application::NODE_TYPE_VIEW, + MenuItem::TARGET_TABLE => Application::NODE_TYPE_TABLE, + default => null, + }; + if ($nodeType === null || !is_int($targetId)) { + return null; + } + return [sprintf('t%di%d', $nodeType, $targetId), [$nodeType, $targetId]]; + } + + /** + * A new target must exist and be manageable by the user, and every unusable one gets the + * same answer, so the save reveals nothing about others' views and tables. A target that was + * in the stored menu before and no longer exists is kept; the menu marks it broken. * * @param list $menuItems + * @param list> $storedMenu * @throws Exception|PermissionError */ - private function addMenuTargetsAsNodes(Context $context, array $menuItems, ?string $userId): void { + private function addMenuTargetsAsNodes(Context $context, array $menuItems, array $storedMenu, ?string $userId): void { $missing = []; foreach ($menuItems as $item) { - $nodeType = match ($item->getTargetType()) { - MenuItem::TARGET_VIEW => Application::NODE_TYPE_VIEW, - MenuItem::TARGET_TABLE => Application::NODE_TYPE_TABLE, - default => null, - }; - $nodeId = $item->getTargetId(); - if ($nodeType !== null && $nodeId !== null) { - $missing[sprintf('t%di%d', $nodeType, $nodeId)] = [$nodeType, $nodeId]; + $target = self::menuTargetNode($item->getTargetType(), $item->getTargetId()); + if ($target !== null) { + $missing[$target[0]] = $target[1]; + } + } + $storedTargets = []; + foreach ($storedMenu as $storedItem) { + $target = self::menuTargetNode($storedItem['targetType'] ?? null, $storedItem['targetId'] ?? null); + if ($target !== null) { + $storedTargets[$target[0]] = true; } } $nodes = $context->getNodes() ?? []; @@ -272,11 +337,12 @@ private function addMenuTargetsAsNodes(Context $context, array $menuItems, ?stri } } - foreach ($missing as [$nodeType, $nodeId]) { - if (!isset($existing[$nodeType][$nodeId])) { + foreach ($missing as $key => [$nodeType, $nodeId]) { + $exists = isset($existing[$nodeType][$nodeId]); + if (!$exists && isset($storedTargets[$key])) { continue; } - if (!$this->permissionsService->canManageNodeById($nodeType, $nodeId, $userId)) { + if (!$exists || !$this->permissionsService->canManageNodeById($nodeType, $nodeId, $userId)) { throw new PermissionError(sprintf('Owner cannot manage node %d (type %d)', $nodeId, $nodeType)); } $relation = $this->addNodeToContext($context, $nodeId, $nodeType, Application::PERMISSION_READ); diff --git a/openapi.json b/openapi.json index fb790ada33..dbf564a889 100644 --- a/openapi.json +++ b/openapi.json @@ -12488,7 +12488,7 @@ "type": "array", "nullable": true, "default": null, - "description": "provide this parameter to replace the menu; the views and tables it opens become nodes of the context", + "description": "provide this parameter to replace the menu; the views and tables it opens become nodes of the context, and an item without a technicalName keeps the one stored for the same target", "items": { "type": "object", "required": [ diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index e7b097f964..992cc9151b 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -7102,7 +7102,7 @@ export interface operations { readonly order: number; } | null; /** - * @description provide this parameter to replace the menu; the views and tables it opens become nodes of the context + * @description provide this parameter to replace the menu; the views and tables it opens become nodes of the context, and an item without a technicalName keeps the one stored for the same target * @default null */ readonly menuItems?: readonly { diff --git a/tests/unit/Model/MenuItemInputTest.php b/tests/unit/Model/MenuItemInputTest.php index 49c9e1cd0f..fe7a667aac 100644 --- a/tests/unit/Model/MenuItemInputTest.php +++ b/tests/unit/Model/MenuItemInputTest.php @@ -44,6 +44,8 @@ public static function invalidItems(): array { 'id of zero' => [['label' => 'X', 'targetType' => 'table', 'targetId' => 0], 'needs its id'], 'javascript link' => [['label' => 'X', 'url' => 'javascript:alert(1)'], 'http or https'], 'data link' => [['label' => 'X', 'url' => 'data:text/html,hi'], 'http or https'], + // carries a host, so only the scheme check stops it + 'javascript link with a host' => [['label' => 'X', 'url' => 'javascript://example.com/%0Aalert(1)'], 'http or https'], 'link without host' => [['label' => 'X', 'url' => 'https://'], 'http or https'], 'empty link' => [['label' => 'X', 'url' => ''], 'http or https'], 'technical name with a hyphen' => [['label' => 'X', 'url' => 'https://example.org', 'technicalName' => 'my-page'], 'technical name'], diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index d10d323792..e17ea1b14d 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -181,21 +181,36 @@ public function testMenuTargetsBecomeNodesSoRecipientsCanOpenThem(): void { $this->assertSame(200, $updated->getPages()[301]['content'][501]['node_rel_id']); } - public function testAMenuTargetTheOwnerCannotManageIsRefused(): void { + public static function unusableNewTargets(): array { + return [ + 'exists but cannot be managed' => [[4 => new Table()]], + 'does not exist' => [[]], + ]; + } + + /** + * Both get the same answer, so a save reveals nothing about which ids exist. + * + * @dataProvider unusableNewTargets + */ + public function testANewMenuTargetThatCannotBeUsedIsRefusedTheSameWay(array $tables): void { $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); $this->contextMapper->method('update')->willReturnArgument(0); - $this->expectExistingTargets([], [4 => new Table()]); - $this->permissionsService->method('canManageNodeById')->with(Application::NODE_TYPE_TABLE, 4, 'user-1')->willReturn(false); + $this->expectExistingTargets([], $tables); + $this->permissionsService->method('canManageNodeById')->willReturn(false); $this->menuItemMapper->expects($this->never())->method('deleteAllByContextId'); $this->expectException(PermissionError::class); + $this->expectExceptionMessage('Owner cannot manage node 4 (type 0)'); $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ ['label' => 'Requests', 'targetType' => 'table', 'targetId' => 4], ])); } - public function testAMenuTargetThatNoLongerExistsIsKeptWithoutANode(): void { - $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + public function testAStoredMenuTargetThatNoLongerExistsIsKeptWithoutANode(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [ + ['targetType' => 'view', 'targetId' => 99, 'url' => null, 'technicalName' => 'gone'], + ])); $this->contextMapper->method('update')->willReturnArgument(0); $this->expectExistingTargets([], []); $this->contextNodeRelMapper->expects($this->never())->method('insert'); @@ -209,6 +224,29 @@ public function testAMenuTargetThatNoLongerExistsIsKeptWithoutANode(): void { $this->assertSame([99], array_map(static fn (MenuItem $item) => $item->getTargetId(), $inserted)); } + public function testAnItemSentWithoutANameKeepsTheStoredNameOfItsTarget(): void { + $context = $this->contextWithMenu(7, [ + ['targetType' => 'view', 'targetId' => 6, 'url' => null, 'technicalName' => 'home'], + ['targetType' => 'url', 'targetId' => null, 'url' => 'https://example.org/a', 'technicalName' => 'docs'], + ['targetType' => 'url', 'targetId' => null, 'url' => 'https://example.org/b', 'technicalName' => 'docs_2'], + ]); + $context->setNodes([101 => ['id' => 101, 'node_id' => 6, 'node_type' => Application::NODE_TYPE_VIEW, 'permissions' => 1]]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + // the view is renamed, the two links swap places and a new link is added + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Start', 'targetType' => 'view', 'targetId' => 6], + ['label' => 'Docs', 'url' => 'https://example.org/b'], + ['label' => 'Docs', 'url' => 'https://example.org/a'], + ['label' => 'Docs', 'url' => 'https://example.org/c'], + ])); + + $this->assertSame(['home', 'docs_2', 'docs', 'docs_3'], array_map(static fn (MenuItem $item) => $item->getTechnicalName(), $inserted)); + } + public function testDerivedTechnicalNamesGetASuffixWhenTheMenuAlreadyUsesThem(): void { $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); $this->contextMapper->method('update')->willReturnArgument(0); @@ -253,7 +291,10 @@ public function testCreateStoresTheMenuItems(): void { $context->setId(8); return $context; }); - $this->pageMapper->method('insert')->willReturnArgument(0); + $this->pageMapper->method('insert')->willReturnCallback(static function (Page $page): Page { + $page->setId(3); + return $page; + }); $inserted = []; $this->expectMenuReplacement(8, $inserted); From 0299c48d21bd50c5a536b5749f7ba0104402b362 Mon Sep 17 00:00:00 2001 From: Remko Date: Tue, 13 Oct 2026 11:48:26 +0200 Subject: [PATCH 3/6] fix(contexts): retry an application update that loses its write lock The update runs in one transaction since the menu is saved with it. On SQLite a concurrent request can take the write lock between its read and its write, and the update then failed at once with a 500. It now runs again on a retryable database error, as atomicRetry() does in core. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Service/ContextService.php | 4 +++- tests/unit/Service/ContextServiceTest.php | 13 +++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 6fe5553c19..8ebb595630 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -378,7 +378,9 @@ protected function technicalNameFromLabel(string $label): string { * @throws PermissionError|MultipleObjectsReturnedException|InvalidArgumentException */ public function update(int $contextId, string $userId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $menuItems = null): Context { - return $this->atomic( + // A transaction that reads before it writes can lose a write lock to a concurrent request + // (SQLite answers "database is locked" at once); running it again then succeeds + return $this->atomicRetry( fn (): Context => $this->applyUpdate($contextId, $userId, $name, $iconName, $description, $nodes, $menuItems), $this->dbc, ); diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index e17ea1b14d..520ac51f7d 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -10,6 +10,7 @@ namespace OCA\Tables\Service; use InvalidArgumentException; +use OC\DB\Exceptions\DbalException; use OCA\Tables\AppInfo\Application; use OCA\Tables\Db\Context; use OCA\Tables\Db\ContextMapper; @@ -274,6 +275,18 @@ public function testTwoMenuItemsWithTheSameTechnicalNameAreRefused(): void { ])); } + public function testUpdateRunsAgainWhenTheDatabaseAsksForARetry(): void { + $locked = $this->createMock(DbalException::class); + $locked->method('isRetryable')->willReturn(true); + $this->contextMapper->expects($this->exactly(2))->method('findById') + ->willReturnOnConsecutiveCalls($this->throwException($locked), $this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + + $updated = $this->service->update(7, 'user-1', 'Renamed', null, null, null); + + $this->assertSame('Renamed', $updated->getName()); + } + public function testUpdateLeavesTheMenuAloneWhenNoneIsGiven(): void { $context = $this->contextWithMenu(7, [['id' => 1, 'label' => 'Keep me']]); $this->contextMapper->method('findById')->willReturn($context); From be888f1293439919df4f953dbfdeafbf6ee153b9 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 15 Oct 2026 11:22:10 +0200 Subject: [PATCH 4/6] feat(menu): save page targets and give new applications a dashboard item A saved page target must be a page of the same application, otherwise the save is refused with a 400; it adds no node. Creating an application puts a "Dashboard" item first that opens its new start page. A page item sent without an id on create opens that start page; on update it is refused. Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/Controller/ContextController.php | 6 +- lib/Model/MenuItemInput.php | 32 ++++- lib/Service/ContextService.php | 91 ++++++++++-- openapi.json | 8 +- src/types/openapi/openapi.ts | 6 +- .../unit/Controller/ContextControllerTest.php | 72 ++++++++++ tests/unit/Model/MenuItemInputTest.php | 23 +++ tests/unit/Service/ContextServiceTest.php | 135 +++++++++++++++++- 8 files changed, 342 insertions(+), 31 deletions(-) create mode 100644 tests/unit/Controller/ContextControllerTest.php diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index ac236b95ae..fdc58d2e27 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -101,7 +101,7 @@ public function show(int $contextId): DataResponse { * @param string $iconName Material design icon name of the context * @param string $description Descriptive text of the context * @psalm-param list $nodes optional nodes to be connected to this context - * @psalm-param ?list $menuItems optional menu of the context, in order; the views and tables it opens become nodes of the context + * @psalm-param ?list $menuItems optional menu of the context, in order; the views and tables it opens become nodes of the context, a page item without a targetId opens the start page, and an item opening the start page is put first unless the menu has one * * @return DataResponse|DataResponse * @@ -122,7 +122,7 @@ public function create(string $name, string $iconName, string $description = '', $this->sanitizeInputNodes($nodes), $this->userId, 0, - $menuItems !== null ? MenuItemInput::createListFromInputArray($menuItems) : null, + $menuItems !== null ? MenuItemInput::createListFromInputArray($menuItems, allowNewStartPage: true) : null, )->jsonSerialize()); } catch (Exception $e) { return $this->handleError($e); @@ -141,7 +141,7 @@ public function create(string $name, string $iconName, string $description = '', * @param ?string $iconName provide this parameter to set a new icon * @param ?string $description provide this parameter to set a new description * @param ?array{id: int, type: int, permissions: int, order: int} $nodes provide this parameter to set a new list of nodes. - * @psalm-param ?list $menuItems provide this parameter to replace the menu; the views and tables it opens become nodes of the context, and an item without a technicalName keeps the one stored for the same target + * @psalm-param ?list $menuItems provide this parameter to replace the menu; the views and tables it opens become nodes of the context, and an item without a technicalName keeps the one stored for the same target * * @return DataResponse|DataResponse * diff --git a/lib/Model/MenuItemInput.php b/lib/Model/MenuItemInput.php index e8e80906f2..c3c0380724 100644 --- a/lib/Model/MenuItemInput.php +++ b/lib/Model/MenuItemInput.php @@ -30,6 +30,7 @@ public function __construct( private readonly ?int $targetId, private readonly ?string $url, private readonly ?string $technicalName, + private readonly bool $allowNewStartPage = false, ) { if (trim($this->label) === '' || mb_strlen($this->label) > self::MAX_LABEL_LENGTH) { throw new InvalidArgumentException('A menu item needs a label of at most ' . self::MAX_LABEL_LENGTH . ' characters.'); @@ -42,7 +43,7 @@ public function __construct( } if ($this->targetType === MenuItem::TARGET_URL) { self::assertUrlValid($this->url); - } elseif ($this->targetId === null || $this->targetId <= 0) { + } elseif (!$this->pointsAtNewStartPage() && ($this->targetId === null || $this->targetId <= 0)) { throw new InvalidArgumentException('A menu item pointing at a ' . $this->targetType . ' needs its id.'); } if ($this->technicalName !== null) { @@ -51,9 +52,11 @@ public function __construct( } /** + * @param bool $allowNewStartPage accept a page item without an id, meaning the start page of + * an application that is being created * @throws InvalidArgumentException */ - public static function createFromInputArray(mixed $data): self { + public static function createFromInputArray(mixed $data, bool $allowNewStartPage = false): self { if (!is_array($data)) { throw new InvalidArgumentException('A menu item must be an object.'); } @@ -66,7 +69,8 @@ public static function createFromInputArray(mixed $data): self { } $isUrl = $targetType === MenuItem::TARGET_URL; $targetId = $data['targetId'] ?? null; - if (!$isUrl && !is_int($targetId)) { + $isNewStartPage = $allowNewStartPage && $targetType === MenuItem::TARGET_PAGE && $targetId === null; + if (!$isUrl && !$isNewStartPage && !is_int($targetId)) { throw new InvalidArgumentException('A menu item pointing at a ' . $targetType . ' needs its id.'); } foreach (['icon', 'url', 'technicalName'] as $optional) { @@ -82,18 +86,36 @@ public static function createFromInputArray(mixed $data): self { targetId: $isUrl ? null : $targetId, url: $isUrl ? trim((string)($data['url'] ?? '')) : null, technicalName: self::emptyToNull($data['technicalName'] ?? null), + allowNewStartPage: $allowNewStartPage, ); } /** + * @param bool $allowNewStartPage see createFromInputArray() * @return list * @throws InvalidArgumentException */ - public static function createListFromInputArray(mixed $data): array { + public static function createListFromInputArray(mixed $data, bool $allowNewStartPage = false): array { if (!is_array($data) || !array_is_list($data)) { throw new InvalidArgumentException('The menu items must be a list.'); } - return array_map(self::createFromInputArray(...), $data); + return array_map(static fn (mixed $item): self => self::createFromInputArray($item, $allowNewStartPage), $data); + } + + /** + * True for a page item without an id on a menu of an application that is being created. + */ + public function pointsAtNewStartPage(): bool { + return $this->allowNewStartPage && $this->targetType === MenuItem::TARGET_PAGE && $this->targetId === null; + } + + /** + * This item pointing at the given page. + * + * @throws InvalidArgumentException + */ + public function withPageId(int $pageId): self { + return new self($this->label, $this->icon, MenuItem::TARGET_PAGE, $pageId, null, $this->technicalName); } /** diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 33bc18e9a4..73d6720c7d 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -39,6 +39,7 @@ use OCP\DB\Exception; use OCP\EventDispatcher\IEventDispatcher; use OCP\IDBConnection; +use OCP\IL10N; use OCP\INavigationManager; use OCP\IURLGenerator; use OCP\IUserManager; @@ -66,6 +67,7 @@ public function __construct( private TableMapper $tableMapper, private ViewMapper $viewMapper, private MenuItemMapper $menuItemMapper, + private IL10N $l10n, ) { } @@ -154,22 +156,93 @@ public function create(string $name, string $iconName, string $description, arra $this->insertNodesFromArray($context, $nodes); } $this->insertPage($context); - $this->replaceMenu($context, $menuItems ?? [], $ownerId); + $startPageId = self::startPageId($context->getPages() ?? []); + $menu = $menuItems ?? []; + if ($startPageId !== null) { + $menu = $this->menuWithStartPage($menu, $startPageId); + } + $this->replaceMenu($context, $menu, $ownerId); }, $this->dbc); return $context; } + /** + * The menu of a new application: a page item without an id opens the start page, and a + * "Dashboard" item opening the start page goes first unless the menu already opens it. + * + * @param list $menuItems + * @return list + * @throws InvalidArgumentException + */ + private function menuWithStartPage(array $menuItems, int $startPageId): array { + $opensStartPage = false; + $taken = []; + foreach ($menuItems as $position => $item) { + if ($item->pointsAtNewStartPage()) { + $item = $menuItems[$position] = $item->withPageId($startPageId); + } + if ($item->getTargetType() === MenuItem::TARGET_PAGE && $item->getTargetId() === $startPageId) { + $opensStartPage = true; + } + if ($item->getTechnicalName() !== null) { + $taken[$item->getTechnicalName()] = true; + } + } + if ($opensStartPage) { + return $menuItems; + } + $dashboard = new MenuItemInput( + label: $this->l10n->t('Dashboard'), + icon: null, + targetType: MenuItem::TARGET_PAGE, + targetId: $startPageId, + url: null, + technicalName: self::uniqueTechnicalName('dashboard', $taken), + ); + return [$dashboard, ...$menuItems]; + } + + /** + * @param array $pages + */ + private static function startPageId(array $pages): ?int { + foreach ($pages as $pageId => $page) { + if (is_array($page) && ($page['page_type'] ?? $page['pageType'] ?? null) === Page::TYPE_STARTPAGE) { + return (int)$pageId; + } + } + return null; + } + + /** + * @param array $taken + */ + private static function uniqueTechnicalName(string $base, array $taken): string { + $technicalName = $base; + for ($suffix = 2; isset($taken[$technicalName]); $suffix++) { + $technicalName = substr($base, 0, MenuItemInput::MAX_TECHNICAL_NAME_LENGTH - 10) . '_' . $suffix; + } + return $technicalName; + } + /** * Replaces the menu of an application and adds the views and tables it points at * as nodes, so everyone the application is shared with can open them. Removing an * item leaves its view or table in the application: it stays a resource until it is - * removed as one. + * removed as one. A page item must open a page of this application; it adds no node. * * @param list $menuItems * @throws Exception|PermissionError|InvalidArgumentException */ protected function replaceMenu(Context $context, array $menuItems, ?string $userId): void { + $pageIds = array_map(intval(...), array_keys($context->getPages() ?? [])); + foreach ($menuItems as $item) { + if ($item->getTargetType() === MenuItem::TARGET_PAGE && !in_array($item->getTargetId(), $pageIds, true)) { + throw new InvalidArgumentException('A menu item can only open a page of its own application.'); + } + } + $storedMenu = array_values(array_filter($context->getMenuItems() ?? [], is_array(...))); $this->addMenuTargetsAsNodes($context, $menuItems, $storedMenu, $userId); @@ -235,11 +308,7 @@ private function menuTechnicalNames(array $menuItems, array $storedMenu): array if (isset($names[$position])) { continue; } - $base = $this->technicalNameFromLabel($item->getLabel()); - $technicalName = $base; - for ($suffix = 2; isset($taken[$technicalName]); $suffix++) { - $technicalName = substr($base, 0, MenuItemInput::MAX_TECHNICAL_NAME_LENGTH - 10) . '_' . $suffix; - } + $technicalName = self::uniqueTechnicalName($this->technicalNameFromLabel($item->getLabel()), $taken); $names[$position] = $technicalName; $taken[$technicalName] = true; } @@ -330,13 +399,7 @@ private function addMenuTargetsAsNodes(Context $context, array $menuItems, array ]; $pages = $context->getPages() ?? []; - $startPageId = null; - foreach ($pages as $pageId => $page) { - if (($page['page_type'] ?? $page['pageType'] ?? null) === Page::TYPE_STARTPAGE) { - $startPageId = (int)$pageId; - break; - } - } + $startPageId = self::startPageId($pages); foreach ($missing as $key => [$nodeType, $nodeId]) { $exists = isset($existing[$nodeType][$nodeId]); diff --git a/openapi.json b/openapi.json index 14eb3389ea..da2d0a029d 100644 --- a/openapi.json +++ b/openapi.json @@ -12082,7 +12082,7 @@ "type": "array", "nullable": true, "default": null, - "description": "optional menu of the context, in order; the views and tables it opens become nodes of the context", + "description": "optional menu of the context, in order; the views and tables it opens become nodes of the context, a page item without a targetId opens the start page, and an item opening the start page is put first unless the menu has one", "items": { "type": "object", "required": [ @@ -12101,7 +12101,8 @@ "enum": [ "view", "table", - "url" + "url", + "page" ] }, "targetId": { @@ -12572,7 +12573,8 @@ "enum": [ "view", "table", - "url" + "url", + "page" ] }, "targetId": { diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index e5ad32e3d8..ad49019dc4 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -6920,14 +6920,14 @@ export interface operations { readonly permissions?: number; }[]; /** - * @description optional menu of the context, in order; the views and tables it opens become nodes of the context + * @description optional menu of the context, in order; the views and tables it opens become nodes of the context, a page item without a targetId opens the start page, and an item opening the start page is put first unless the menu has one * @default null */ readonly menuItems?: readonly { readonly label: string; readonly icon?: string; /** @enum {string} */ - readonly targetType: "view" | "table" | "url"; + readonly targetType: "view" | "table" | "url" | "page"; /** Format: int64 */ readonly targetId?: number; readonly url?: string; @@ -7131,7 +7131,7 @@ export interface operations { readonly label: string; readonly icon?: string; /** @enum {string} */ - readonly targetType: "view" | "table" | "url"; + readonly targetType: "view" | "table" | "url" | "page"; /** Format: int64 */ readonly targetId?: number; readonly url?: string; diff --git a/tests/unit/Controller/ContextControllerTest.php b/tests/unit/Controller/ContextControllerTest.php new file mode 100644 index 0000000000..2bd2f5cd65 --- /dev/null +++ b/tests/unit/Controller/ContextControllerTest.php @@ -0,0 +1,72 @@ +contextService = $this->createMock(ContextService::class); + $this->controller = new ContextController( + $this->createMock(IRequest::class), + $this->createMock(LoggerInterface::class), + $this->createMock(IL10N::class), + 'user-1', + $this->contextService, + $this->createMock(TableService::class), + $this->createMock(IDBConnection::class), + $this->createMock(ColumnService::class), + $this->createMock(ViewService::class), + ); + } + + public function testCreatePassesAPageItemWithoutAnIdOnAsTheNewStartPage(): void { + $context = new Context(); + $context->setMenuItems([]); + $this->contextService->expects($this->once())->method('create') + ->with('Intake', 'star', '', [], 'user-1', 0, $this->callback(static function (array $menuItems): bool { + return count($menuItems) === 1 + && $menuItems[0] instanceof MenuItemInput + && $menuItems[0]->pointsAtNewStartPage() + && $menuItems[0]->getLabel() === 'Home'; + })) + ->willReturn($context); + + $response = $this->controller->create('Intake', 'star', '', [], [['label' => 'Home', 'targetType' => 'page']]); + + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + } + + public function testUpdateRefusesAPageItemWithoutAnId(): void { + $this->contextService->expects($this->never())->method('update'); + + $response = $this->controller->update(7, null, null, null, null, [['label' => 'Home', 'targetType' => 'page']]); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + $this->assertStringContainsString('needs its id', $response->getData()['message']); + } +} diff --git a/tests/unit/Model/MenuItemInputTest.php b/tests/unit/Model/MenuItemInputTest.php index fe7a667aac..b0915c2d60 100644 --- a/tests/unit/Model/MenuItemInputTest.php +++ b/tests/unit/Model/MenuItemInputTest.php @@ -40,6 +40,7 @@ public static function invalidItems(): array { 'label too long' => [['label' => str_repeat('x', 201), 'url' => 'https://example.org'], 'needs a label'], 'unknown target type' => [['label' => 'X', 'targetType' => 'kanban', 'targetId' => 1], 'Unexpected menu item target type'], 'view without id' => [['label' => 'X', 'targetType' => 'view'], 'needs its id'], + 'page without id' => [['label' => 'X', 'targetType' => 'page'], 'needs its id'], 'id as a string' => [['label' => 'X', 'targetType' => 'table', 'targetId' => '4'], 'needs its id'], 'id of zero' => [['label' => 'X', 'targetType' => 'table', 'targetId' => 0], 'needs its id'], 'javascript link' => [['label' => 'X', 'url' => 'javascript:alert(1)'], 'http or https'], @@ -62,6 +63,28 @@ public function testRejectsInvalidInput(mixed $input, string $message): void { MenuItemInput::createFromInputArray($input); } + public function testAPageItemWithoutAnIdOpensTheNewStartPageOnlyWhenAllowed(): void { + [$startPage, $page] = MenuItemInput::createListFromInputArray([ + ['label' => 'Dashboard', 'targetType' => 'page', 'technicalName' => 'home'], + ['label' => 'Overview', 'targetType' => 'page', 'targetId' => 5], + ], allowNewStartPage: true); + + $this->assertTrue($startPage->pointsAtNewStartPage()); + $this->assertSame(['Dashboard', null, 'page', null, null, 'home'], $this->toArray($startPage)); + $this->assertFalse($page->pointsAtNewStartPage()); + $this->assertSame(['Overview', null, 'page', 5, null, null], $this->toArray($page)); + + $resolved = $startPage->withPageId(3); + $this->assertFalse($resolved->pointsAtNewStartPage()); + $this->assertSame(['Dashboard', null, 'page', 3, null, 'home'], $this->toArray($resolved)); + } + + public function testOnlyAPageItemMayLeaveOutItsIdWhenTheStartPageIsAllowed(): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('needs its id'); + MenuItemInput::createFromInputArray(['label' => 'X', 'targetType' => 'view'], allowNewStartPage: true); + } + public function testRejectsAMenuThatIsNotAList(): void { $this->expectException(InvalidArgumentException::class); $this->expectExceptionMessage('must be a list'); diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index 520ac51f7d..b5cf0ad342 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -30,6 +30,7 @@ use OCA\Tables\Model\MenuItemInput; use OCP\EventDispatcher\IEventDispatcher; use OCP\IDBConnection; +use OCP\IL10N; use OCP\INavigationManager; use OCP\IURLGenerator; use OCP\IUserManager; @@ -66,6 +67,8 @@ protected function setUp(): void { $shareService = $this->createMock(ShareService::class); $navigationManager = $this->createMock(INavigationManager::class); $urlGenerator = $this->createMock(IURLGenerator::class); + $l10n = $this->createMock(IL10N::class); + $l10n->method('t')->willReturnCallback(static fn (string $text): string => 'translated ' . $text); $this->service = new ContextService( $this->contextMapper, @@ -84,6 +87,7 @@ protected function setUp(): void { $this->tableMapper, $this->viewMapper, $this->menuItemMapper, + $l10n, ); } @@ -313,11 +317,136 @@ public function testCreateStoresTheMenuItems(): void { $created = $this->service->create('Intake portal', 'heart', ' desc ', [], 'user-1', 0, MenuItemInput::createListFromInputArray([ ['label' => 'Docs', 'targetType' => 'url', 'url' => 'https://example.org'], - ])); + ], allowNewStartPage: true)); $this->assertSame('desc', $created->getDescription()); - $this->assertCount(1, $created->getMenuItems()); - $this->assertSame('docs', $inserted[0]->getTechnicalName()); + $this->assertCount(2, $created->getMenuItems()); + $this->assertSame(['dashboard', 'docs'], array_map(static fn (MenuItem $item) => $item->getTechnicalName(), $inserted)); + } + + /** + * @param list $inserted collects the stored items + */ + private function expectCreation(int $contextId, int $startPageId, array &$inserted): void { + $this->contextMapper->method('insert')->willReturnCallback(static function (Context $context) use ($contextId): Context { + $context->setId($contextId); + return $context; + }); + $this->pageMapper->method('insert')->willReturnCallback(static function (Page $page) use ($startPageId): Page { + $page->setId($startPageId); + return $page; + }); + $this->expectMenuReplacement($contextId, $inserted); + } + + /** + * @param list $items + * @return list + */ + private static function describeMenu(array $items): array { + return array_map(static fn (MenuItem $item): array => [ + $item->getLabel(), $item->getTargetType(), $item->getTargetId(), $item->getTechnicalName(), $item->getIcon(), + ], $items); + } + + public function testCreateWithoutAMenuAddsADashboardItemForTheStartPage(): void { + $inserted = []; + $this->expectCreation(8, 3, $inserted); + + $created = $this->service->create('Intake portal', 'heart', '', [], 'user-1', 0); + + $this->assertSame([['translated Dashboard', 'page', 3, 'dashboard', null]], self::describeMenu($inserted)); + $this->assertSame(3, $created->getMenuItems()[0]['targetId']); + } + + public function testCreatePutsTheDashboardItemFirstWithAFreeTechnicalName(): void { + $inserted = []; + $this->expectCreation(8, 3, $inserted); + + $this->service->create('Intake portal', 'heart', '', [], 'user-1', 0, MenuItemInput::createListFromInputArray([ + ['label' => 'Docs', 'url' => 'https://example.org', 'technicalName' => 'dashboard'], + ], allowNewStartPage: true)); + + $this->assertSame([ + ['translated Dashboard', 'page', 3, 'dashboard_2', null], + ['Docs', 'url', null, 'dashboard', null], + ], self::describeMenu($inserted)); + } + + public function testCreateResolvesAPageItemWithoutAnIdToTheStartPage(): void { + $inserted = []; + $this->expectCreation(8, 3, $inserted); + + $this->service->create('Intake portal', 'heart', '', [], 'user-1', 0, MenuItemInput::createListFromInputArray([ + ['label' => 'Docs', 'url' => 'https://example.org'], + ['label' => 'Overview', 'targetType' => 'page', 'icon' => 'home'], + ], allowNewStartPage: true)); + + $this->assertSame([ + ['Docs', 'url', null, 'docs', null], + ['Overview', 'page', 3, 'overview', 'home'], + ], self::describeMenu($inserted)); + } + + public function testAPageOfTheApplicationIsAcceptedAndAddsNoNode(): void { + $context = $this->contextWithMenu(7, []); + $context->setPages([301 => ['id' => 301, 'page_type' => Page::TYPE_STARTPAGE, 'content' => []]]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->contextNodeRelMapper->expects($this->never())->method('insert'); + $this->pageContentMapper->expects($this->never())->method('insert'); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $updated = $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Dashboard', 'targetType' => 'page', 'targetId' => 301], + ])); + + $this->assertSame([['Dashboard', 'page', 301, 'dashboard', null]], self::describeMenu($inserted)); + $this->assertSame([], $updated->getNodes()); + } + + public function testAPageOfAnotherApplicationIsRefused(): void { + $context = $this->contextWithMenu(7, []); + $context->setPages([301 => ['id' => 301, 'page_type' => Page::TYPE_STARTPAGE, 'content' => []]]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->menuItemMapper->expects($this->never())->method('deleteAllByContextId'); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('a page of its own application'); + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Elsewhere', 'targetType' => 'page', 'targetId' => 302], + ])); + } + + public function testUpdateRefusesAPageItemWithoutAnId(): void { + $context = $this->contextWithMenu(7, []); + $context->setPages([301 => ['id' => 301, 'page_type' => Page::TYPE_STARTPAGE, 'content' => []]]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->menuItemMapper->expects($this->never())->method('deleteAllByContextId'); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('a page of its own application'); + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Dashboard', 'targetType' => 'page'], + ], allowNewStartPage: true)); + } + + public function testUpdateAddsNoDashboardItem(): void { + $context = $this->contextWithMenu(7, []); + $context->setPages([301 => ['id' => 301, 'page_type' => Page::TYPE_STARTPAGE, 'content' => []]]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Docs', 'url' => 'https://example.org'], + ])); + + $this->assertSame([['Docs', 'url', null, 'docs', null]], self::describeMenu($inserted)); } public function testDeleteRemovesTheMenuOfTheApplication(): void { From cff25544a3ec926b7ebe1cf23be6fb5336cd66a7 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 15 Oct 2026 11:58:24 +0200 Subject: [PATCH 5/6] fix(menu): keep a stored page item whose page is gone Saving a menu no longer fails on an item that points at a page that was in the stored menu but no longer exists; it is kept and stays broken, like a view or table. A page target that is new in the save must still be a page of the application. Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/Service/ContextService.php | 11 +++++- tests/unit/Service/ContextServiceTest.php | 47 +++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 73d6720c7d..6b61bd9970 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -230,20 +230,27 @@ private static function uniqueTechnicalName(string $base, array $taken): string * Replaces the menu of an application and adds the views and tables it points at * as nodes, so everyone the application is shared with can open them. Removing an * item leaves its view or table in the application: it stays a resource until it is - * removed as one. A page item must open a page of this application; it adds no node. + * removed as one. A new page target must be a page of this + * application; a stored one whose page is gone is kept, and the menu marks it broken. A page + * item adds no node. * * @param list $menuItems * @throws Exception|PermissionError|InvalidArgumentException */ protected function replaceMenu(Context $context, array $menuItems, ?string $userId): void { + $storedMenu = array_values(array_filter($context->getMenuItems() ?? [], is_array(...))); $pageIds = array_map(intval(...), array_keys($context->getPages() ?? [])); + foreach ($storedMenu as $storedItem) { + if (($storedItem['targetType'] ?? null) === MenuItem::TARGET_PAGE && is_int($storedItem['targetId'] ?? null)) { + $pageIds[] = $storedItem['targetId']; + } + } foreach ($menuItems as $item) { if ($item->getTargetType() === MenuItem::TARGET_PAGE && !in_array($item->getTargetId(), $pageIds, true)) { throw new InvalidArgumentException('A menu item can only open a page of its own application.'); } } - $storedMenu = array_values(array_filter($context->getMenuItems() ?? [], is_array(...))); $this->addMenuTargetsAsNodes($context, $menuItems, $storedMenu, $userId); $this->menuItemMapper->deleteAllByContextId($context->getId()); diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index b5cf0ad342..3bc6c42c5a 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -420,6 +420,53 @@ public function testAPageOfAnotherApplicationIsRefused(): void { ])); } + public function testAStoredPageItemWhosePageIsGoneIsKept(): void { + $context = $this->contextWithMenu(7, [ + ['targetType' => 'page', 'targetId' => 302, 'url' => null, 'technicalName' => 'overview', 'broken' => true], + ]); + $context->setPages([301 => ['id' => 301, 'page_type' => Page::TYPE_STARTPAGE, 'content' => []]]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->contextNodeRelMapper->expects($this->never())->method('insert'); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Overview', 'targetType' => 'page', 'targetId' => 302], + ['label' => 'Docs', 'url' => 'https://example.org'], + ])); + + $this->assertSame([['Overview', 'page', 302, 'overview', null], ['Docs', 'url', null, 'docs', null]], self::describeMenu($inserted)); + } + + public static function newPageTargetsOutsideTheApplication(): array { + return [ + 'page of another application' => [302], + 'page id that does not exist' => [9999], + ]; + } + + /** + * @dataProvider newPageTargetsOutsideTheApplication + */ + public function testANewPageTargetOutsideTheApplicationIsRefusedEvenWithABrokenStoredOne(int $pageId): void { + // the stored item points at a page that is gone; that does not let other pages in + $context = $this->contextWithMenu(7, [ + ['targetType' => 'page', 'targetId' => 303, 'url' => null, 'technicalName' => 'gone', 'broken' => true], + ]); + $context->setPages([301 => ['id' => 301, 'page_type' => Page::TYPE_STARTPAGE, 'content' => []]]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->menuItemMapper->expects($this->never())->method('deleteAllByContextId'); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('a page of its own application'); + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Gone', 'targetType' => 'page', 'targetId' => 303], + ['label' => 'Elsewhere', 'targetType' => 'page', 'targetId' => $pageId], + ])); + } + public function testUpdateRefusesAPageItemWithoutAnId(): void { $context = $this->contextWithMenu(7, []); $context->setPages([301 => ['id' => 301, 'page_type' => Page::TYPE_STARTPAGE, 'content' => []]]); From 1cd65a0182113f2feec93b93105b21973b19b5bc Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 16 Oct 2026 09:26:24 +0200 Subject: [PATCH 6/6] feat(menu): store a submitted menu of a new application as it is Creating an application adds the "Dashboard" item for the start page only when no menu is sent. A submitted menu, even an empty one, is stored as submitted, so a start-page item removed in the create dialog stays removed. A page item without an id still opens the new start page. Refs nextcloud/tables#3099 Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/Controller/ContextController.php | 2 +- lib/Service/ContextService.php | 39 +++++++++-------------- openapi.json | 2 +- src/types/openapi/openapi.ts | 2 +- tests/unit/Service/ContextServiceTest.php | 19 ++++++++--- 5 files changed, 33 insertions(+), 31 deletions(-) diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index fdc58d2e27..0489b1ba5b 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -101,7 +101,7 @@ public function show(int $contextId): DataResponse { * @param string $iconName Material design icon name of the context * @param string $description Descriptive text of the context * @psalm-param list $nodes optional nodes to be connected to this context - * @psalm-param ?list $menuItems optional menu of the context, in order; the views and tables it opens become nodes of the context, a page item without a targetId opens the start page, and an item opening the start page is put first unless the menu has one + * @psalm-param ?list $menuItems optional menu of the context, in order; the views and tables it opens become nodes of the context, a page item without a targetId opens the start page; without a menu, the context gets one item opening the start page * * @return DataResponse|DataResponse * diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 6b61bd9970..0a23c8e9bf 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -159,7 +159,7 @@ public function create(string $name, string $iconName, string $description, arra $startPageId = self::startPageId($context->getPages() ?? []); $menu = $menuItems ?? []; if ($startPageId !== null) { - $menu = $this->menuWithStartPage($menu, $startPageId); + $menu = $menuItems === null ? [$this->dashboardItem($startPageId)] : self::menuWithStartPage($menuItems, $startPageId); } $this->replaceMenu($context, $menu, $ownerId); }, $this->dbc); @@ -168,39 +168,30 @@ public function create(string $name, string $iconName, string $description, arra } /** - * The menu of a new application: a page item without an id opens the start page, and a - * "Dashboard" item opening the start page goes first unless the menu already opens it. + * The submitted menu of a new application, where a page item without an id opens the start page. * * @param list $menuItems * @return list - * @throws InvalidArgumentException */ - private function menuWithStartPage(array $menuItems, int $startPageId): array { - $opensStartPage = false; - $taken = []; - foreach ($menuItems as $position => $item) { - if ($item->pointsAtNewStartPage()) { - $item = $menuItems[$position] = $item->withPageId($startPageId); - } - if ($item->getTargetType() === MenuItem::TARGET_PAGE && $item->getTargetId() === $startPageId) { - $opensStartPage = true; - } - if ($item->getTechnicalName() !== null) { - $taken[$item->getTechnicalName()] = true; - } - } - if ($opensStartPage) { - return $menuItems; - } - $dashboard = new MenuItemInput( + private static function menuWithStartPage(array $menuItems, int $startPageId): array { + return array_map( + static fn (MenuItemInput $item): MenuItemInput => $item->pointsAtNewStartPage() ? $item->withPageId($startPageId) : $item, + $menuItems, + ); + } + + /** + * The "Dashboard" item opening the start page, the menu of a new application created without one. + */ + private function dashboardItem(int $startPageId): MenuItemInput { + return new MenuItemInput( label: $this->l10n->t('Dashboard'), icon: null, targetType: MenuItem::TARGET_PAGE, targetId: $startPageId, url: null, - technicalName: self::uniqueTechnicalName('dashboard', $taken), + technicalName: 'dashboard', ); - return [$dashboard, ...$menuItems]; } /** diff --git a/openapi.json b/openapi.json index da2d0a029d..4a70afef1f 100644 --- a/openapi.json +++ b/openapi.json @@ -12082,7 +12082,7 @@ "type": "array", "nullable": true, "default": null, - "description": "optional menu of the context, in order; the views and tables it opens become nodes of the context, a page item without a targetId opens the start page, and an item opening the start page is put first unless the menu has one", + "description": "optional menu of the context, in order; the views and tables it opens become nodes of the context, a page item without a targetId opens the start page; without a menu, the context gets one item opening the start page", "items": { "type": "object", "required": [ diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index ad49019dc4..0ac2af9067 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -6920,7 +6920,7 @@ export interface operations { readonly permissions?: number; }[]; /** - * @description optional menu of the context, in order; the views and tables it opens become nodes of the context, a page item without a targetId opens the start page, and an item opening the start page is put first unless the menu has one + * @description optional menu of the context, in order; the views and tables it opens become nodes of the context, a page item without a targetId opens the start page; without a menu, the context gets one item opening the start page * @default null */ readonly menuItems?: readonly { diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index 3bc6c42c5a..f0c6e761ab 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -320,8 +320,8 @@ public function testCreateStoresTheMenuItems(): void { ], allowNewStartPage: true)); $this->assertSame('desc', $created->getDescription()); - $this->assertCount(2, $created->getMenuItems()); - $this->assertSame(['dashboard', 'docs'], array_map(static fn (MenuItem $item) => $item->getTechnicalName(), $inserted)); + $this->assertCount(1, $created->getMenuItems()); + $this->assertSame(['docs'], array_map(static fn (MenuItem $item) => $item->getTechnicalName(), $inserted)); } /** @@ -359,17 +359,28 @@ public function testCreateWithoutAMenuAddsADashboardItemForTheStartPage(): void $this->assertSame(3, $created->getMenuItems()[0]['targetId']); } - public function testCreatePutsTheDashboardItemFirstWithAFreeTechnicalName(): void { + public function testCreateWithAnEmptyMenuStoresNoItems(): void { + $inserted = []; + $this->expectCreation(8, 3, $inserted); + + $created = $this->service->create('Intake portal', 'heart', '', [], 'user-1', 0, []); + + $this->assertSame([], $inserted); + $this->assertSame([], $created->getMenuItems()); + } + + public function testCreateStoresAMenuWithoutAStartPageItemAsSubmitted(): void { $inserted = []; $this->expectCreation(8, 3, $inserted); $this->service->create('Intake portal', 'heart', '', [], 'user-1', 0, MenuItemInput::createListFromInputArray([ ['label' => 'Docs', 'url' => 'https://example.org', 'technicalName' => 'dashboard'], + ['label' => 'Guide', 'url' => 'https://example.org/guide'], ], allowNewStartPage: true)); $this->assertSame([ - ['translated Dashboard', 'page', 3, 'dashboard_2', null], ['Docs', 'url', null, 'dashboard', null], + ['Guide', 'url', null, 'guide', null], ], self::describeMenu($inserted)); }