From fe28163f725119ede8b7d4e0047a0da25a91bb1b Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 22 Oct 2026 15:10:24 +0200 Subject: [PATCH 1/2] feat(menu): save the menu of an application Creating and updating an application can include its menu, saved as a whole. Links must be http or https, page targets must be pages of the same application, and tables and views in the menu become part of the application. A new application without a submitted menu gets a Dashboard item that opens its start page. Co-authored-by: remko48 Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ruben van der Linde --- lib/Controller/ContextController.php | 17 +- lib/Db/ContextNodeRelation.php | 15 + lib/Db/MenuItem.php | 10 + lib/Db/MenuItemMapper.php | 2 +- lib/Db/PageContent.php | 13 + lib/Model/MenuItemInput.php | 178 ++++++ lib/Service/ContextService.php | 344 +++++++++++- .../unit/Controller/ContextControllerTest.php | 121 ++++ tests/unit/Db/MenuItemTest.php | 7 + tests/unit/Model/MenuItemInputTest.php | 109 ++++ .../Service/ContextPagesRoundTripTest.php | 77 ++- tests/unit/Service/ContextServiceTest.php | 526 +++++++++++++++++- 12 files changed, 1402 insertions(+), 17 deletions(-) create mode 100644 lib/Model/MenuItemInput.php create mode 100644 tests/unit/Controller/ContextControllerTest.php create mode 100644 tests/unit/Model/MenuItemInputTest.php diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index 434e2137bb..0184e28da9 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -17,6 +17,7 @@ use OCA\Tables\Errors\PermissionError; use OCA\Tables\Helper\IconHelper; 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; @@ -101,6 +102,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 and 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 * @@ -109,7 +111,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); @@ -121,13 +123,16 @@ public function create(string $name, string $iconName, string $description = '', $this->sanitizeInputNodes($nodes), $this->userId, 0, + $menuItems !== null ? MenuItemInput::createListFromInputArray($menuItems, allowNewStartPage: true) : null, )->jsonSerialize()); } catch (Exception $e) { return $this->handleError($e); } catch (PermissionError $e) { return $this->handlePermissionError($e); + } catch (BadRequestError $e) { + return $this->handleBadRequestError($e); } catch (InvalidArgumentException $e) { - return $this->handleError(new InternalError($e->getMessage(), $e->getCode(), $e)); + return $this->handleBadRequestError(new BadRequestError($e->getMessage(), $e->getCode(), $e)); } } @@ -139,6 +144,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 * * @return DataResponse|DataResponse * @@ -150,7 +156,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); @@ -163,6 +169,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); @@ -170,6 +177,10 @@ 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 (BadRequestError $e) { + return $this->handleBadRequestError($e); + } catch (InvalidArgumentException $e) { + return $this->handleBadRequestError(new BadRequestError($e->getMessage(), $e->getCode(), $e)); } } diff --git a/lib/Db/ContextNodeRelation.php b/lib/Db/ContextNodeRelation.php index fbabd3e4da..3f8d7bda9e 100644 --- a/lib/Db/ContextNodeRelation.php +++ b/lib/Db/ContextNodeRelation.php @@ -35,6 +35,21 @@ public function __construct() { $this->addType('permissions', 'integer'); } + /** + * The relation as an application carries it among its nodes, in the shape the + * context mapper reads it. + * + * @return array{id: int, node_id: int, node_type: int, permissions: int} + */ + public function toContextNode(): array { + return [ + 'id' => (int)$this->getId(), + 'node_id' => (int)$this->getNodeId(), + 'node_type' => (int)$this->getNodeType(), + 'permissions' => (int)$this->getPermissions(), + ]; + } + public function jsonSerialize(): array { return [ 'id' => $this->getId(), diff --git a/lib/Db/MenuItem.php b/lib/Db/MenuItem.php index 9866e7af6e..3dd92ab78c 100644 --- a/lib/Db/MenuItem.php +++ b/lib/Db/MenuItem.php @@ -44,6 +44,8 @@ class MenuItem extends EntitySuper implements JsonSerializable { public const TARGET_URL = 'url'; public const TARGET_PAGE = 'page'; public const TARGET_TYPES = [self::TARGET_VIEW, self::TARGET_TABLE, self::TARGET_URL, self::TARGET_PAGE]; + public const DEFAULT_NAME_PREFIX = 'item_'; + public const MAX_ITEMS_PER_APPLICATION = 200; protected ?int $contextId = null; protected ?string $label = null; @@ -70,6 +72,14 @@ public function __construct() { $this->addType('broken', 'boolean'); } + /** + * Whether a technical name has the form of the automatic `item_`. Such names are + * reserved, so a given name never collides with the name a later item gets. + */ + public static function isDefaultName(string $technicalName): bool { + return preg_match('/^' . self::DEFAULT_NAME_PREFIX . '[0-9]+$/', $technicalName) === 1; + } + /** * @return array{id: int, contextId: int, label: string, icon: string|null, description: string|null, targetType: 'view'|'table'|'url'|'page', targetId: int|null, url: string|null, technicalName: string, order: int, parentId: int|null, broken: bool} */ diff --git a/lib/Db/MenuItemMapper.php b/lib/Db/MenuItemMapper.php index 618bd0ddf2..704e9c4722 100644 --- a/lib/Db/MenuItemMapper.php +++ b/lib/Db/MenuItemMapper.php @@ -35,7 +35,7 @@ public function __construct(IDBConnection $db) { public function insertWithDefaultName(MenuItem $menuItem): MenuItem { return $this->atomic(function () use ($menuItem): MenuItem { $menuItem = $this->insert($menuItem); - $menuItem->setTechnicalName('item_' . $menuItem->getId()); + $menuItem->setTechnicalName(MenuItem::DEFAULT_NAME_PREFIX . $menuItem->getId()); return $this->update($menuItem); }, $this->db); } diff --git a/lib/Db/PageContent.php b/lib/Db/PageContent.php index 966f48fc53..518aacfcce 100644 --- a/lib/Db/PageContent.php +++ b/lib/Db/PageContent.php @@ -30,6 +30,19 @@ public function __construct() { $this->addType('order', 'integer'); } + /** + * The content as a page of an application carries it, in the shape the context + * mapper reads it. + * + * @return array{order: int, node_rel_id: int} + */ + public function toPageContent(): array { + return [ + 'order' => (int)$this->getOrder(), + 'node_rel_id' => (int)$this->getNodeRelId(), + ]; + } + public function jsonSerialize(): array { return [ 'id' => $this->getId(), diff --git a/lib/Model/MenuItemInput.php b/lib/Model/MenuItemInput.php new file mode 100644 index 0000000000..21a4c00719 --- /dev/null +++ b/lib/Model/MenuItemInput.php @@ -0,0 +1,178 @@ +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 && !IconHelper::isValid($this->icon)) { + throw new InvalidArgumentException('A menu item has an unknown icon: ' . $this->icon); + } + if ($this->description !== null && mb_strlen($this->description) > self::MAX_DESCRIPTION_LENGTH) { + throw new InvalidArgumentException('A menu item description has at most ' . self::MAX_DESCRIPTION_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->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 && !TechnicalNameValidator::isValid($this->technicalName)) { + throw new InvalidArgumentException('A technical name has 1 to ' . TechnicalNameValidator::MAX_LENGTH . ' lower case letters, digits and underscores and starts with a letter.'); + } + } + + /** + * @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, bool $allowNewStartPage = false): 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; + $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', 'description', '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), + description: self::emptyToNull($data['description'] ?? null), + targetType: $targetType, + 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, 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(static fn (mixed $item): self => self::createFromInputArray($item, $allowNewStartPage), $data); + } + + /** + * True for a page item without an id on the 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, $this->description, MenuItem::TARGET_PAGE, $pageId, null, $this->technicalName); + } + + /** + * @throws InvalidArgumentException + */ + private static function assertUrlValid(?string $url): void { + $scheme = $url === null ? null : parse_url($url, PHP_URL_SCHEME); + // parse_url accepts spaces and control characters anywhere in a link + if ($url === null || mb_strlen($url) > self::MAX_URL_LENGTH || preg_match('/[\s\p{Z}\p{Cc}]/u', $url) !== 0 + || !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 getDescription(): ?string { + return $this->description; + } + + 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 d8dce10913..2842f9a241 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -31,6 +31,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 OCA\Tables\Model\ViewSettings; use OCA\Tables\Validation\TechnicalNameValidator; @@ -40,6 +41,7 @@ use OCP\DB\Exception; use OCP\EventDispatcher\IEventDispatcher; use OCP\IDBConnection; +use OCP\IL10N; use OCP\INavigationManager; use OCP\IURLGenerator; use OCP\IUserManager; @@ -69,6 +71,7 @@ public function __construct( private WidgetService $widgetService, private GridWidgetTypes $gridWidgetTypes, private MenuItemMapper $menuItemMapper, + private IL10N $l10n, ) { } @@ -180,10 +183,15 @@ private function markMenuForReader(Context $context, ?string $userId): void { } /** + * Creates an application with its start page. Without a menu it gets a "Dashboard" + * item opening the start page; a given menu, even an empty one, is stored as it is, + * where a page item without an id opens the start page. + * * @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)); @@ -191,7 +199,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)) { @@ -199,19 +207,335 @@ public function create(string $name, string $iconName, string $description, arra $this->insertNodesFromArray($context, $nodes); } $this->insertPage($context); + $startPageId = self::startPageId($context->getPages() ?? []); + $menu = $menuItems ?? []; + if ($startPageId !== null) { + $menu = $menuItems === null + ? [$this->dashboardItem($startPageId)] + : array_map(static fn (MenuItemInput $item): MenuItemInput => $item->pointsAtNewStartPage() ? $item->withPageId($startPageId) : $item, $menuItems); + } + $this->replaceMenu($context, $menu, $ownerId); }, $this->dbc); + $this->shapeAddedForResponse($context, [], [], true); return $context; } + /** + * Puts the nodes and start page contents a save added into the shape the application + * endpoints have always returned for them: a node as its relation and a content entry + * as its row. While saving, every lookup uses the shape of the context mapper. + * + * @param array $knownNodeIds relation ids the application had before the save + * @param array $knownContentIds content ids the application had before the save + * @param bool $isNew a new application lists its nodes and leaves the page id out of its contents + */ + private function shapeAddedForResponse(Context $context, array $knownNodeIds, array $knownContentIds, bool $isNew): void { + $nodes = $context->getNodes(); + if (is_array($nodes)) { + foreach ($nodes as $relId => $node) { + if (!isset($knownNodeIds[$relId]) && isset($node['node_id'])) { + $nodes[$relId] = [ + 'id' => $node['id'], + 'contextId' => $context->getId(), + 'nodeId' => $node['node_id'], + 'nodeType' => $node['node_type'], + 'permissions' => $node['permissions'], + ]; + } + } + $context->setNodes($isNew ? array_values($nodes) : $nodes); + } + $pages = $context->getPages(); + if (is_array($pages)) { + foreach ($pages as $pageId => $page) { + foreach ($page['content'] ?? [] as $contentId => $content) { + if (isset($knownContentIds[$contentId]) || !isset($content['node_rel_id'])) { + continue; + } + $pages[$pageId]['content'][$contentId] = $isNew + ? ['id' => $contentId, 'nodeRelId' => $content['node_rel_id'], 'order' => $content['order']] + : ['id' => $contentId, 'pageId' => $pageId, 'nodeRelId' => $content['node_rel_id'], 'order' => $content['order']]; + } + } + $context->setPages($pages); + } + } + + /** + * 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, + description: null, + targetType: MenuItem::TARGET_PAGE, + targetId: $startPageId, + url: null, + technicalName: Page::START_PAGE_NAME, + ); + } + + /** + * @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; + } + + /** + * Replaces the menu of an application and adds the views and tables it opens as nodes, + * so everyone the application is shared with can open them. Removing an item leaves + * its view or table in the application. A page item must open a page of this + * application; a stored one whose page is gone is kept and read as broken. + * + * @param list $menuItems + * @throws Exception|PermissionError|InvalidArgumentException + */ + protected function replaceMenu(Context $context, array $menuItems, ?string $userId): void { + if (count($menuItems) > MenuItem::MAX_ITEMS_PER_APPLICATION) { + throw new InvalidArgumentException('An application menu holds at most ' . MenuItem::MAX_ITEMS_PER_APPLICATION . ' items.'); + } + $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.'); + } + } + + $keptNames = $this->keptTechnicalNames($menuItems, $storedMenu); + $this->addMenuTargetsAsNodes($context, $menuItems, $storedMenu, $userId); + + $this->menuItemMapper->deleteAllByContextId($context->getId()); + $this->insertMenuItems($context->getId(), $menuItems, $keptNames); + + $context->setMenuItems(array_map( + static fn (MenuItem $menuItem): array => $menuItem->jsonSerialize(), + $this->menuItemMapper->findByContextId($context->getId()), + )); + } + + /** + * Inserts the items of a menu in their order. An item with a kept technical name is + * saved under it and refused when another save took it; every other item is named + * `item_`. + * + * @param list $menuItems + * @param array $keptNames technical names by position + * @throws Exception|InvalidArgumentException + */ + private function insertMenuItems(int $contextId, array $menuItems, array $keptNames): void { + foreach ($menuItems as $position => $item) { + $menuItem = new MenuItem(); + $menuItem->setContextId($contextId); + $menuItem->setLabel($item->getLabel()); + $menuItem->setIcon($item->getIcon()); + $menuItem->setDescription($item->getDescription()); + $menuItem->setTargetType($item->getTargetType()); + $menuItem->setTargetId($item->getTargetId()); + $menuItem->setUrl($item->getUrl()); + $menuItem->setOrder(($position + 1) * 10); + + if (!isset($keptNames[$position])) { + $this->menuItemMapper->insertWithDefaultName($menuItem); + continue; + } + $menuItem->setTechnicalName($keptNames[$position]); + try { + $this->menuItemMapper->insert($menuItem); + } catch (Exception $e) { + if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + throw $e; + } + throw new InvalidArgumentException('The technical name ' . $keptNames[$position] . ' is already used by a menu item of this application.'); + } + } + } + + /** + * The technical names items keep, by position: the one given, else the stored name + * of an item with the same target, so renaming or moving an item keeps it. + * + * @param list $menuItems + * @param list> $storedMenu + * @return array + * @throws InvalidArgumentException when two items are given the same name or a given + * name has the automatic form and is not the item's stored one + */ + private function keptTechnicalNames(array $menuItems, array $storedMenu): array { + $storedNames = []; + 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; + $storedNames[self::menuTargetIdentity($targetType, $targetId, $url)][] = $technicalName; + } + + $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 . '".'); + } + $identity = self::menuTargetIdentity($item->getTargetType(), $item->getTargetId(), $item->getUrl()); + if (MenuItem::isDefaultName($technicalName) && !in_array($technicalName, $storedNames[$identity] ?? [], true)) { + throw new InvalidArgumentException('The technical name ' . $technicalName . ' is reserved for automatic names.'); + } + $names[$position] = $technicalName; + $taken[$technicalName] = true; + } + + 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; + } + } + } + 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]]; + } + + /** + * Adds the views and tables a menu opens to the application and its start page. A + * new target must exist and be manageable by the user, and every unusable one gets + * the same answer, so a save reveals nothing about the views and tables of others. A + * target that was in the stored menu before and is gone is kept. + * + * @param list $menuItems + * @param list> $storedMenu + * @throws Exception|PermissionError + */ + private function addMenuTargetsAsNodes(Context $context, array $menuItems, array $storedMenu, ?string $userId): void { + $missing = []; + foreach ($menuItems as $item) { + $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() ?? []; + foreach ($nodes as $node) { + unset($missing[sprintf('t%di%d', $node['node_type'], $node['node_id'])]); + } + 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 = self::startPageId($pages); + foreach ($missing as $key => [$nodeType, $nodeId]) { + $exists = isset($existing[$nodeType][$nodeId]); + if (!$exists && isset($storedTargets[$key])) { + continue; + } + 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); + $nodes[$relation->getId()] = $relation->toContextNode(); + if ($startPageId !== null) { + $content = $this->addNodeRelToPage($relation, null, $startPageId); + $pages[$startPageId]['content'][$content->getId()] = $content->toPageContent(); + } + } + $context->setNodes($nodes); + $context->setPages($pages); + } + /** * @psalm-param list $nodes * @throws Exception * @throws DoesNotExistException * @throws PermissionError|MultipleObjectsReturnedException + * @param list|null $menuItems replaces the menu when given + * @throws 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 { + // A transaction that reads before it writes can lose its write lock to a concurrent + // request (SQLite answers "database is locked" at once); running it again succeeds. + return $this->atomicRetry( + fn (): Context => $this->applyUpdate($contextId, $userId, $name, $iconName, $description, $nodes, $menuItems), + $this->dbc, + ); + } + + /** + * @psalm-param list $nodes + * @throws Exception + * @throws DoesNotExistException + * @throws PermissionError|MultipleObjectsReturnedException + * @param list|null $menuItems + */ + private function applyUpdate(int $contextId, string $userId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $menuItems): Context { $context = $this->contextMapper->findById($contextId, $userId); + $knownNodeIds = array_fill_keys(array_keys($context->getNodes() ?? []), true); + $knownContentIds = []; + foreach ($context->getPages() ?? [] as $page) { + $knownContentIds += array_fill_keys(array_keys($page['content'] ?? []), true); + } if ($name !== null) { $context->setName(trim($name)); @@ -293,8 +617,8 @@ public function update(int $contextId, string $userId, ?string $name, ?string $i $node['order'] ?? 100, $userId ); - $currentNodes[$addedNode->getId()] = $addedNode->jsonSerialize(); - $currentPages[$updatedContent->getPageId()]['content'][$updatedContent->getId()] = $updatedContent->jsonSerialize(); + $currentNodes[$addedNode->getId()] = $addedNode->toContextNode(); + $currentPages[$updatedContent->getPageId()]['content'][$updatedContent->getId()] = $updatedContent->toPageContent(); } unset($nodesBeingAdded); @@ -354,7 +678,11 @@ 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); + } + $this->shapeAddedForResponse($context, $knownNodeIds, $knownContentIds, false); return $context; } @@ -599,9 +927,7 @@ protected function insertPage(Context $context): void { $this->pageContentMapper->insert($pageContent); - $addedPage['content'][$pageContent->getId()] = $pageContent->jsonSerialize(); - // the content is already embedded in the page - unset($addedPage['content'][$pageContent->getId()]['pageId']); + $addedPage['content'][$pageContent->getId()] = $pageContent->toPageContent(); } } @@ -625,7 +951,7 @@ protected function insertNodesFromArray(Context $context, array $nodes): void { throw new PermissionError(sprintf('Owner cannot manage node %d (type %d)', $nodeId, $nodeType)); } $contextNodeRel = $this->addNodeToContext($context, $nodeId, $nodeType, $node['permissions'] ?? Application::PERMISSION_READ); - $addedNodes[] = $contextNodeRel->jsonSerialize(); + $addedNodes[$contextNodeRel->getId()] = $contextNodeRel->toContextNode(); } catch (Exception $e) { $this->logger->warning('Could not add node {ntype}/{nid} to context {cid}, skipping.', [ 'app' => Application::APP_ID, diff --git a/tests/unit/Controller/ContextControllerTest.php b/tests/unit/Controller/ContextControllerTest.php new file mode 100644 index 0000000000..e89f008a30 --- /dev/null +++ b/tests/unit/Controller/ContextControllerTest.php @@ -0,0 +1,121 @@ +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 fn (array $menuItems): bool => 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']); + } + + public function testCreateWithoutAMenuLeavesItToTheService(): void { + $context = new Context(); + $context->setMenuItems([]); + $this->contextService->expects($this->once())->method('create') + ->with('Intake', 'star', '', [], 'user-1', 0, null) + ->willReturn($context); + + $this->assertSame(Http::STATUS_OK, $this->controller->create('Intake', 'star')->getStatus()); + } + + public static function invalidMenus(): array { + return [ + 'not a list' => [['home' => ['label' => 'Home', 'url' => 'https://example.org']], 'must be a list'], + 'javascript link' => [[['label' => 'X', 'url' => 'javascript:alert(1)']], 'http or https'], + 'unknown icon' => [[['label' => 'X', 'url' => 'https://example.org', 'icon' => 'no-such-icon']], 'unknown icon'], + ]; + } + + /** + * @dataProvider invalidMenus + */ + public function testAnInvalidMenuIsABadRequest(array $menuItems, string $message): void { + $this->contextService->expects($this->never())->method('create'); + $this->contextService->expects($this->never())->method('update'); + + foreach ([$this->controller->create('Intake', 'star', '', [], $menuItems), $this->controller->update(7, null, null, null, null, $menuItems)] as $response) { + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + $this->assertStringContainsString($message, $response->getData()['message']); + } + } + + public function testUpdatePassesTheMenu(): void { + $context = new Context(); + $context->setMenuItems([]); + $this->contextService->expects($this->once())->method('update') + ->with(7, 'user-1', null, null, null, null, $this->callback(static fn (array $menuItems): bool => count($menuItems) === 1 && $menuItems[0]->getTargetId() === 5)) + ->willReturn($context); + + $response = $this->controller->update(7, null, null, null, null, [['label' => 'Overview', 'targetType' => 'page', 'targetId' => 5]]); + + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + } + + public function testATakenTechnicalNameIsABadRequest(): void { + $this->contextService->method('update')->willThrowException(new \InvalidArgumentException('The technical name docs is already used by a menu item of this application.')); + + $response = $this->controller->update(7, null, null, null, null, [['label' => 'Docs', 'url' => 'https://example.org', 'technicalName' => 'docs']]); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + } +} diff --git a/tests/unit/Db/MenuItemTest.php b/tests/unit/Db/MenuItemTest.php index 2bf5ba417b..d58f198d1f 100644 --- a/tests/unit/Db/MenuItemTest.php +++ b/tests/unit/Db/MenuItemTest.php @@ -89,4 +89,11 @@ public function testBrokenIsReportedButNeverStored(): void { public function testTargetTypesAreTheFourKnownOnes(): void { $this->assertSame(['view', 'table', 'url', 'page'], MenuItem::TARGET_TYPES); } + + public function testOnlyTheAutomaticFormIsADefaultName(): void { + $this->assertTrue(MenuItem::isDefaultName('item_7')); + foreach (['item', 'item_', 'item_7a', 'my_item_7', 'page_7', 'docs'] as $name) { + $this->assertFalse(MenuItem::isDefaultName($name), $name); + } + } } diff --git a/tests/unit/Model/MenuItemInputTest.php b/tests/unit/Model/MenuItemInputTest.php new file mode 100644 index 0000000000..8ce6a356ca --- /dev/null +++ b/tests/unit/Model/MenuItemInputTest.php @@ -0,0 +1,109 @@ + ' Intake home ', 'targetType' => 'view', 'targetId' => 6, 'technicalName' => 'home', 'url' => 'https://ignored.example', 'description' => ' Where intake starts '], + ['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', 'Where intake starts'], $this->toArray($items[0])); + $this->assertSame(['Docs', 'book', 'url', null, 'https://example.org/docs', null, null], $this->toArray($items[1])); + $this->assertSame(['Requests', null, 'table', 4, null, 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'], + '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'], + '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'], + 'link with a space' => [['label' => 'X', 'url' => 'https://exa mple.com'], 'http or https'], + 'link with a tab' => [['label' => 'X', 'url' => "https://example.com/\tpath"], 'http or https'], + 'link with a line break' => [['label' => 'X', 'url' => "https://example.com/a\nb"], 'http or https'], + 'link with a control character' => [['label' => 'X', 'url' => "https://example.com/\x01"], 'http or https'], + 'link with a delete character' => [['label' => 'X', 'url' => "https://example.com/\x7f"], 'http or https'], + 'link with a non-breaking space' => [['label' => 'X', 'url' => "https://example.com/\u{00A0}x"], 'http or https'], + 'link that is not utf-8' => [['label' => 'X', 'url' => "https://example.com/\xff"], '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'], + 'unknown icon' => [['label' => 'X', 'url' => 'https://example.org', 'icon' => 'no-such-icon'], 'unknown icon'], + 'description that is not a string' => [['label' => 'X', 'url' => 'https://example.org', 'description' => 3], '"description" must be a string'], + 'description too long' => [['label' => 'X', 'url' => 'https://example.org', 'description' => str_repeat('x', 2001)], 'description has at most'], + 'technical name with a capital' => [['label' => 'X', 'url' => 'https://example.org', 'technicalName' => 'Home'], 'technical name'], + 'technical name too long' => [['label' => 'X', 'url' => 'https://example.org', 'technicalName' => str_repeat('a', 201)], 'technical name'], + ]; + } + + /** + * @dataProvider invalidItems + */ + public function testRejectsInvalidInput(mixed $input, string $message): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage($message); + 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', null], $this->toArray($startPage)); + $this->assertFalse($page->pointsAtNewStartPage()); + $this->assertSame(['Overview', null, 'page', 5, null, null, null], $this->toArray($page)); + + $resolved = $startPage->withPageId(3); + $this->assertFalse($resolved->pointsAtNewStartPage()); + $this->assertSame(['Dashboard', null, 'page', 3, null, 'home', null], $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'); + 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(), $item->getDescription()]; + } +} diff --git a/tests/unit/Service/ContextPagesRoundTripTest.php b/tests/unit/Service/ContextPagesRoundTripTest.php index d2800fafad..c6664e646e 100644 --- a/tests/unit/Service/ContextPagesRoundTripTest.php +++ b/tests/unit/Service/ContextPagesRoundTripTest.php @@ -25,6 +25,7 @@ use OCA\Tables\Db\WidgetSourceMapper; use OCA\Tables\Helper\GroupHelper; use OCA\Tables\Helper\UserHelper; +use OCA\Tables\Model\MenuItemInput; use OCA\Tables\Service\ContextService; use OCA\Tables\Service\GridWidgetTypes; use OCA\Tables\Service\PermissionsService; @@ -36,6 +37,7 @@ use OCP\INavigationManager; use OCP\IURLGenerator; use OCP\IUserManager; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; /** @@ -50,6 +52,8 @@ class ContextPagesRoundTripTest extends DatabaseTestCase { private ContextNodeRelationMapper $nodeRelMapper; private PageMapper $pageMapper; private WidgetService $widgetService; + private PermissionsService&MockObject $permissionsService; + private TableMapper&MockObject $tableMapper; /** @var int[] */ private array $contextIds = []; @@ -72,7 +76,7 @@ protected function setUp(): void { $this->pageMapper, new PageContentMapper($this->connectionAdapter), $this->createMock(LoggerInterface::class), - $this->createMock(PermissionsService::class), + $this->permissionsService = $this->createMock(PermissionsService::class), $this->createMock(IUserManager::class), $this->createMock(IEventDispatcher::class), $this->connectionAdapter, @@ -80,11 +84,12 @@ protected function setUp(): void { false, $this->createMock(INavigationManager::class), $this->createMock(IURLGenerator::class), - $this->createMock(TableMapper::class), + $this->tableMapper = $this->createMock(TableMapper::class), $this->createMock(ViewMapper::class), $this->widgetService, new GridWidgetTypes($l10n), new MenuItemMapper($this->connectionAdapter), + $l10n, ); } @@ -197,4 +202,72 @@ private function createContextWithGridPage(): array { return [$context, $relation, $page->getId()]; } + + /** + * The frontend sends a table both as a node and as a menu target. + */ + public function testATableInBothTheNodesAndTheMenuIsAddedOnceOnCreateAndUpdate(): void { + $this->permissionsService->method('canManageNodeById')->willReturn(true); + $this->tableMapper->method('findMany')->willReturnCallback(static fn (array $ids): array => array_combine($ids, array_map(static fn (): Table => new Table(), $ids))); + $node = static fn (int $id): array => ['id' => $id, 'type' => Application::NODE_TYPE_TABLE, 'permissions' => Application::PERMISSION_READ]; + $item = static fn (int $id): array => ['label' => 'Table ' . $id, 'targetType' => 'table', 'targetId' => $id]; + + $created = $this->service->create('Nodes and menu', 'tables', '', [$node(990501)], self::OWNER, Application::OWNER_TYPE_USER, + MenuItemInput::createListFromInputArray([$item(990501)], allowNewStartPage: true)); + $this->contextIds[] = $created->getId(); + $this->assertSame([990501], $this->storedNodeIds($created->getId())); + + $this->service->update($created->getId(), self::OWNER, null, null, null, [$node(990501), $node(990502)], + MenuItemInput::createListFromInputArray([$item(990501), $item(990502)])); + $this->assertSame([990501, 990502], $this->storedNodeIds($created->getId())); + } + + /** + * The node ids of an application, once for every relation, and the start page holds + * each relation once. + * + * @return list + */ + private function storedNodeIds(int $contextId): array { + $context = $this->contextMapper->findById($contextId, self::OWNER); + $nodeIds = array_values(array_map(static fn (array $node): int => $node['node_id'], $context->getNodes())); + sort($nodeIds); + $startPage = array_values(array_filter($context->getPages(), static fn (array $page): bool => $page['page_type'] === Page::TYPE_STARTPAGE))[0]; + $this->assertCount(count($nodeIds), $startPage['content']); + return $nodeIds; + } + + /** + * The responses of creating and updating an application keep the shape they had + * before menus: a new application lists its nodes as relations and its start page + * contents by id, and an update returns a node it added as its relation. + */ + public function testCreateAndUpdateResponsesKeepTheirNodeShape(): void { + $this->permissionsService->method('canManageNodeById')->willReturn(true); + $node = static fn (int $id, int $permissions): array => ['id' => $id, 'type' => Application::NODE_TYPE_TABLE, 'permissions' => $permissions]; + + $created = $this->service->create('Shape', 'tables', '', [$node(990601, 1), $node(990602, 3)], self::OWNER, Application::OWNER_TYPE_USER); + $this->contextIds[] = $id = $created->getId(); + $json = json_decode(json_encode($created), true); + [$first, $second] = $json['nodes']; + $this->assertSame(array_keys($json['nodes']), [0, 1]); + $this->assertSame(['id' => $first['id'], 'contextId' => $id, 'nodeId' => 990601, 'nodeType' => 0, 'permissions' => 1], $first); + $this->assertSame(['id' => $second['id'], 'contextId' => $id, 'nodeId' => 990602, 'nodeType' => 0, 'permissions' => 3], $second); + $startPage = array_values($json['pages'])[0]; + $this->assertSame(['id', 'contextId', 'pageType', 'technicalName', 'title', 'icon', 'description', 'content'], array_keys($startPage)); + $contentIds = array_keys($startPage['content']); + $this->assertSame([ + ['id' => $contentIds[0], 'nodeRelId' => $first['id'], 'order' => 10], + ['id' => $contentIds[1], 'nodeRelId' => $second['id'], 'order' => 20], + ], array_values($startPage['content'])); + + $updated = json_decode(json_encode($this->service->update($id, self::OWNER, null, null, null, [$node(990602, 1), $node(990603, 1)])), true); + $this->assertSame([$second['id']], array_values(array_diff(array_keys($updated['nodes']), [max(array_keys($updated['nodes']))]))); + $added = $updated['nodes'][max(array_keys($updated['nodes']))]; + $this->assertSame(['id', 'contextId', 'nodeId', 'nodeType', 'permissions'], array_keys($added)); + $this->assertSame(['id', 'node_id', 'node_type', 'permissions'], array_keys($updated['nodes'][$second['id']])); + $addedContent = array_values(array_filter(array_values($updated['pages'])[0]['content'], static fn (array $content): bool => isset($content['nodeRelId']))); + $this->assertSame(['id', 'pageId', 'nodeRelId', 'order'], array_keys($addedContent[0])); + $this->assertSame($added['id'], $addedContent[0]['nodeRelId']); + } } diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index 057c94205b..f6a2427fb1 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -9,10 +9,14 @@ 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; 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; @@ -20,7 +24,11 @@ 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 OCA\Tables\Tests\Unit\Database\DbErrorStub; use OCP\EventDispatcher\IEventDispatcher; use OCP\IDBConnection; use OCP\IL10N; @@ -42,6 +50,8 @@ final class ContextServiceTest extends TestCase { private MockObject $permissionsService; private MockObject $shareService; private MockObject $dbConnection; + private MockObject $tableMapper; + private MockObject $viewMapper; protected function setUp(): void { parent::setUp(); @@ -52,6 +62,8 @@ protected function setUp(): void { $this->pageContentMapper = $this->createMock(PageContentMapper::class); $logger = $this->createMock(LoggerInterface::class); $this->permissionsService = $this->createMock(PermissionsService::class); + $this->tableMapper = $this->createMock(TableMapper::class); + $this->viewMapper = $this->createMock(ViewMapper::class); $userManager = $this->createMock(IUserManager::class); $eventDispatcher = $this->createMock(IEventDispatcher::class); $this->dbConnection = $this->createMock(IDBConnection::class); @@ -59,6 +71,8 @@ protected function setUp(): void { $this->menuItemMapper = $this->createMock(MenuItemMapper::class); $l10n = $this->createMock(IL10N::class); $l10n->method('t')->willReturnArgument(0); + $translations = $this->createMock(IL10N::class); + $translations->method('t')->willReturnCallback(static fn (string $text): string => 'translated ' . $text); $this->shareService = $this->createMock(ShareService::class); $navigationManager = $this->createMock(INavigationManager::class); $urlGenerator = $this->createMock(IURLGenerator::class); @@ -77,11 +91,12 @@ protected function setUp(): void { false, $navigationManager, $urlGenerator, - $this->createMock(TableMapper::class), - $this->createMock(ViewMapper::class), + $this->tableMapper, + $this->viewMapper, $this->widgetService, new GridWidgetTypes($l10n), $this->menuItemMapper, + $translations, ); } @@ -457,6 +472,483 @@ public function testAnImportedStartPageLeavesDashboardToThePageExportedWithIt(): self::assertSame([80 => 'page_80', 81 => 'dashboard'], $names); } + 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->id = count($inserted) + 1; + $inserted[] = $item; + return $item; + }); + $this->menuItemMapper->method('insertWithDefaultName')->willReturnCallback(static function (MenuItem $item) use (&$inserted): MenuItem { + $item->id = count($inserted) + 1; + $item->setTechnicalName('item_' . $item->getId()); + $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->id = 200 + count($relations); + $relations[] = $relation; + return $relation; + }); + } + + public function testUpdateReplacesTheMenuInOrderAndNamesUnnamedItemsByTheirId(): 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', 'item_2', 'item_3'], 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->id = 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()]); + // a node added by a save comes back as its relation, as the update has always returned it + $this->assertSame(['id' => 200, 'contextId' => 7, 'nodeId' => 6, 'nodeType' => Application::NODE_TYPE_VIEW, 'permissions' => Application::PERMISSION_READ], $updated->getNodes()[200]); + $this->assertSame(200, $updated->getPages()[301]['content'][501]['nodeRelId']); + } + + 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([], $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 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'); + $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 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', 'item_4'], array_map(static fn (MenuItem $item) => $item->getTechnicalName(), $inserted)); + } + + public function testUnnamedItemsAreNamedByTheirIdBesideGivenNames(): 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(['item_1', 'item_2', '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 testAGivenNameOfTheAutomaticFormIsRefused(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [ + ['targetType' => 'url', 'targetId' => null, 'url' => 'https://example.org/a', 'technicalName' => 'item_3'], + ])); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->menuItemMapper->expects($this->never())->method('deleteAllByContextId'); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('The technical name item_3 is reserved for automatic names.'); + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'B', 'url' => 'https://example.org/b', 'technicalName' => 'item_3'], + ])); + } + + public function testAnItemSentBackWithItsStoredAutomaticNameKeepsIt(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [ + ['targetType' => 'url', 'targetId' => null, 'url' => 'https://example.org/a', 'technicalName' => 'item_3'], + ])); + $this->contextMapper->method('update')->willReturnArgument(0); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'A', 'url' => 'https://example.org/a', 'technicalName' => 'item_3'], + ])); + + $this->assertSame(['item_3'], array_map(static fn (MenuItem $item) => $item->getTechnicalName(), $inserted)); + } + + 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); + $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->id = 8; + return $context; + }); + $this->pageMapper->method('insert')->willReturnCallback(static function (Page $page): Page { + $page->id = 3; + return $page; + }); + $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'], + ], allowNewStartPage: true)); + + $this->assertSame('desc', $created->getDescription()); + $this->assertCount(1, $created->getMenuItems()); + $this->assertSame(['item_1'], 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->id = $contextId; + return $context; + }); + $this->pageMapper->method('insert')->willReturnCallback(static function (Page $page) use ($startPageId): Page { + $page->id = $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 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([ + ['Docs', 'url', null, 'dashboard', null], + ['Guide', 'url', null, 'item_2', 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, 'item_1', null], + ['Overview', 'page', 3, 'item_2', '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, 'item_1', 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 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, 'item_2', 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' => []]]); + $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, 'item_1', null]], self::describeMenu($inserted)); + } + + public function testAGivenMenuNameTakenByAConcurrentSaveIsRefused(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->menuItemMapper->expects($this->once())->method('insert')->willThrowException(DbErrorStub::uniqueViolation()); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('already used'); + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Docs', 'url' => 'https://example.org', 'technicalName' => 'docs'], + ])); + } + + public function testMenuItemsKeepTheirIconAndDescription(): 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', 'icon' => 'book', 'description' => 'How it works'], + ])); + + $this->assertSame([['book', 'How it works']], array_map(static fn (MenuItem $item): array => [$item->getIcon(), $item->getDescription()], $inserted)); + } + private function buildPageContent(int $id, int $pageId, int $nodeRelId, int $order): PageContent { $pageContent = new PageContent(); $pageContent->id = $id; @@ -647,4 +1139,34 @@ public function testDeletingAnApplicationRetriesATransientDatabaseError(): void self::assertSame(2, $attempts); } + + public function testAMenuOfMoreThanTheMaximumNumberOfItemsIsRefused(): void { + $context = new Context(); + $context->id = 7; + $context->setNodes([]); + $context->setPages([]); + $context->setMenuItems([]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->menuItemMapper->expects($this->never())->method('deleteAllByContextId'); + $items = array_fill(0, MenuItem::MAX_ITEMS_PER_APPLICATION + 1, ['label' => 'Docs', 'url' => 'https://example.org']); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('at most 200 items'); + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray($items)); + } + + public function testAMenuOfTheMaximumNumberOfItemsIsSaved(): void { + $context = new Context(); + $context->id = 7; + $context->setNodes([]); + $context->setPages([]); + $context->setMenuItems([]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->menuItemMapper->expects($this->exactly(MenuItem::MAX_ITEMS_PER_APPLICATION))->method('insertWithDefaultName'); + $items = array_fill(0, MenuItem::MAX_ITEMS_PER_APPLICATION, ['label' => 'Docs', 'url' => 'https://example.org']); + + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray($items)); + } } From 86bf3127d3f1baae0ee52c4b08a79fb374b34f26 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 22 Oct 2026 15:10:24 +0200 Subject: [PATCH 2/2] chore(api): regenerate the OpenAPI spec and TypeScript types Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ruben van der Linde --- openapi.json | 84 ++++++++++++++++++++++++++++++++++++ src/types/openapi/openapi.ts | 30 +++++++++++++ 2 files changed, 114 insertions(+) diff --git a/openapi.json b/openapi.json index 6cda2d1a84..8f7d4fc15e 100644 --- a/openapi.json +++ b/openapi.json @@ -12308,6 +12308,48 @@ } } } + }, + "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 and 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": [ + "label" + ], + "properties": { + "label": { + "type": "string" + }, + "icon": { + "type": "string" + }, + "description": { + "type": "string" + }, + "targetType": { + "type": "string", + "enum": [ + "view", + "table", + "url", + "page" + ] + }, + "targetId": { + "type": "integer", + "format": "int64" + }, + "url": { + "type": "string" + }, + "technicalName": { + "type": "string" + } + } + } } } } @@ -12740,6 +12782,48 @@ "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, and an item without a technicalName keeps the one stored for the same target", + "items": { + "type": "object", + "required": [ + "label" + ], + "properties": { + "label": { + "type": "string" + }, + "icon": { + "type": "string" + }, + "description": { + "type": "string" + }, + "targetType": { + "type": "string", + "enum": [ + "view", + "table", + "url", + "page" + ] + }, + "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 7c51a21974..2ff592e2ee 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -7029,6 +7029,21 @@ 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 and 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 { + readonly label: string; + readonly icon?: string; + readonly description?: string; + /** @enum {string} */ + readonly targetType?: "view" | "table" | "url" | "page"; + /** Format: int64 */ + readonly targetId?: number; + readonly url?: string; + readonly technicalName?: string; + }[] | null; }; }; }; @@ -7219,6 +7234,21 @@ 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, and an item without a technicalName keeps the one stored for the same target + * @default null + */ + readonly menuItems?: readonly { + readonly label: string; + readonly icon?: string; + readonly description?: string; + /** @enum {string} */ + readonly targetType?: "view" | "table" | "url" | "page"; + /** Format: int64 */ + readonly targetId?: number; + readonly url?: string; + readonly technicalName?: string; + }[] | null; }; }; };