diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index 0184e28da9..0626ba15d1 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -102,7 +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 + * @psalm-param ?list $menuItems optional menu of the context, in order; parentIndex is the position of the parent item, for one level of nesting; 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 * @@ -144,7 +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 + * @psalm-param ?list $menuItems provide this parameter to replace the menu; parentIndex is the position of the parent item, for one level of nesting; 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 21a4c00719..fc012a3ce5 100644 --- a/lib/Model/MenuItemInput.php +++ b/lib/Model/MenuItemInput.php @@ -15,8 +15,8 @@ use OCA\Tables\Validation\TechnicalNameValidator; /** - * A submitted menu item, checked on its own: its label, icon, description, target and - * technical name. Whether a target may be used is up to the application it is saved in. + * A submitted menu item, checked on its own: its label, icon, description, target, + * technical name and the position of its parent. Whether a target may be used is up to the application it is saved in. */ class MenuItemInput { public const MAX_LABEL_LENGTH = 200; @@ -35,6 +35,7 @@ public function __construct( private readonly ?int $targetId, private readonly ?string $url, private readonly ?string $technicalName, + private readonly ?int $parentIndex = null, private readonly bool $allowNewStartPage = false, ) { if (trim($this->label) === '' || mb_strlen($this->label) > self::MAX_LABEL_LENGTH) { @@ -86,6 +87,10 @@ public static function createFromInputArray(mixed $data, bool $allowNewStartPage throw new InvalidArgumentException('The menu item property "' . $optional . '" must be a string.'); } } + $parentIndex = $data['parentIndex'] ?? null; + if ($parentIndex !== null && (!is_int($parentIndex) || $parentIndex < 0)) { + throw new InvalidArgumentException('The parent of a menu item is given as the position of that parent in the menu.'); + } return new self( label: trim($data['label']), @@ -95,11 +100,15 @@ public static function createFromInputArray(mixed $data, bool $allowNewStartPage targetId: $isUrl ? null : $targetId, url: $isUrl ? trim((string)($data['url'] ?? '')) : null, technicalName: self::emptyToNull($data['technicalName'] ?? null), + parentIndex: $parentIndex, allowNewStartPage: $allowNewStartPage, ); } /** + * A menu with one level of nesting: an item may sit under a top-level item of the + * same menu, named by its position. + * * @param bool $allowNewStartPage see createFromInputArray() * @return list * @throws InvalidArgumentException @@ -108,7 +117,14 @@ public static function createListFromInputArray(mixed $data, bool $allowNewStart 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); + $items = array_map(static fn (mixed $item): self => self::createFromInputArray($item, $allowNewStartPage), $data); + foreach ($items as $item) { + $parentIndex = $item->getParentIndex(); + if ($parentIndex !== null && (!isset($items[$parentIndex]) || $items[$parentIndex]->getParentIndex() !== null)) { + throw new InvalidArgumentException('A menu item can only sit under a top-level item of the same menu.'); + } + } + return $items; } /** @@ -124,7 +140,7 @@ public function pointsAtNewStartPage(): bool { * @throws InvalidArgumentException */ public function withPageId(int $pageId): self { - return new self($this->label, $this->icon, $this->description, MenuItem::TARGET_PAGE, $pageId, null, $this->technicalName); + return new self($this->label, $this->icon, $this->description, MenuItem::TARGET_PAGE, $pageId, null, $this->technicalName, $this->parentIndex); } /** @@ -175,4 +191,11 @@ public function getUrl(): ?string { public function getTechnicalName(): ?string { return $this->technicalName; } + + /** + * The position of the parent item in the same menu, or null for a top-level item. + */ + public function getParentIndex(): ?int { + return $this->parentIndex; + } } diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index e4f6a8b4e6..b5679d9dd9 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -334,38 +334,53 @@ protected function replaceMenu(Context $context, array $menuItems, ?string $user } /** - * 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_`. + * Inserts the items of a menu, top-level items first so a child can point at the id + * of its parent; the order keeps the sequence of the menu. 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; + $topLevelIds = []; + foreach ([false, true] as $childPass) { + foreach ($menuItems as $position => $item) { + $parentIndex = $item->getParentIndex(); + if (($parentIndex !== null) !== $childPass) { + continue; + } + if ($parentIndex !== null && !isset($topLevelIds[$parentIndex])) { + throw new InvalidArgumentException('A menu item can only sit under a top-level item of the same menu.'); + } + $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); + $menuItem->setParentId($parentIndex === null ? null : $topLevelIds[$parentIndex]); + + if (isset($keptNames[$position])) { + $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.'); + } + } else { + $this->menuItemMapper->insertWithDefaultName($menuItem); + } + if ($parentIndex === null) { + $topLevelIds[$position] = $menuItem->getId(); } - throw new InvalidArgumentException('The technical name ' . $keptNames[$position] . ' is already used by a menu item of this application.'); } } } diff --git a/openapi.json b/openapi.json index 3165a3b0a1..2991280bef 100644 --- a/openapi.json +++ b/openapi.json @@ -12597,7 +12597,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 and a page item without a targetId opens the start page; without a menu the context gets one item opening the start page", + "description": "optional menu of the context, in order; parentIndex is the position of the parent item, for one level of nesting; 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": [ @@ -12631,6 +12631,10 @@ }, "technicalName": { "type": "string" + }, + "parentIndex": { + "type": "integer", + "format": "int64" } } } @@ -13071,7 +13075,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, and an item without a technicalName keeps the one stored for the same target", + "description": "provide this parameter to replace the menu; parentIndex is the position of the parent item, for one level of nesting; 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": [ @@ -13105,6 +13109,10 @@ }, "technicalName": { "type": "string" + }, + "parentIndex": { + "type": "integer", + "format": "int64" } } } diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index e092878ad8..131dbfb5e9 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -7229,7 +7229,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 and a page item without a targetId opens the start page; without a menu the context gets one item opening the start page + * @description optional menu of the context, in order; parentIndex is the position of the parent item, for one level of nesting; 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 { @@ -7242,6 +7242,8 @@ export interface operations { readonly targetId?: number; readonly url?: string; readonly technicalName?: string; + /** Format: int64 */ + readonly parentIndex?: number; }[] | null; }; }; @@ -7434,7 +7436,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, and an item without a technicalName keeps the one stored for the same target + * @description provide this parameter to replace the menu; parentIndex is the position of the parent item, for one level of nesting; 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 { @@ -7447,6 +7449,8 @@ export interface operations { readonly targetId?: number; readonly url?: string; readonly technicalName?: string; + /** Format: int64 */ + readonly parentIndex?: number; }[] | null; }; }; diff --git a/tests/unit/Model/MenuItemInputTest.php b/tests/unit/Model/MenuItemInputTest.php index 8ce6a356ca..6031fd8124 100644 --- a/tests/unit/Model/MenuItemInputTest.php +++ b/tests/unit/Model/MenuItemInputTest.php @@ -97,6 +97,52 @@ public function testOnlyAPageItemMayLeaveOutItsIdWhenTheStartPageIsAllowed(): vo MenuItemInput::createFromInputArray(['label' => 'X', 'targetType' => 'view'], allowNewStartPage: true); } + public function testKeepsTheParentPositionForOneLevelOfNesting(): void { + $items = MenuItemInput::createListFromInputArray([ + ['label' => 'Cases', 'targetType' => 'table', 'targetId' => 4], + ['label' => 'Open', 'targetType' => 'view', 'targetId' => 6, 'parentIndex' => 0], + ['label' => 'Docs', 'url' => 'https://example.org', 'parentIndex' => null], + ]); + + $this->assertSame([null, 0, null], array_map(static fn (MenuItemInput $item) => $item->getParentIndex(), $items)); + } + + public static function invalidNesting(): array { + return [ + 'under a child' => [[null, 0, 1], 'top-level'], + 'under itself' => [[null, 1], 'top-level'], + 'under a position that does not exist' => [[null, 5], 'top-level'], + 'given as a string' => [[null, '0'], 'position of that parent'], + 'given as a negative number' => [[null, -1], 'position of that parent'], + ]; + } + + /** + * @dataProvider invalidNesting + */ + public function testRejectsInvalidNesting(array $parentIndexes, string $message): void { + $items = array_map( + static fn (mixed $parentIndex, int $position): array => ['label' => 'Item ' . $position, 'url' => 'https://example.org/' . $position, 'parentIndex' => $parentIndex], + $parentIndexes, + array_keys($parentIndexes), + ); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage($message); + MenuItemInput::createListFromInputArray($items); + } + + public function testResolvingTheStartPageKeepsTheParent(): void { + [, $child] = MenuItemInput::createListFromInputArray([ + ['label' => 'Cases', 'targetType' => 'table', 'targetId' => 4], + ['label' => 'Overview', 'targetType' => 'page', 'parentIndex' => 0], + ], allowNewStartPage: true); + + $resolved = $child->withPageId(3); + $this->assertSame(0, $resolved->getParentIndex()); + $this->assertSame(3, $resolved->getTargetId()); + } + 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 962ba1bbdf..0947ea0c68 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -997,6 +997,88 @@ public function testMenuItemsKeepTheirIconAndDescription(): void { $this->assertSame([['book', 'How it works']], array_map(static fn (MenuItem $item): array => [$item->getIcon(), $item->getDescription()], $inserted)); } + public function testUpdateStoresAChildUnderItsParentAndKeepsTheOrder(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->expectExistingTargets([6 => new View()], [4 => new Table()]); + $this->permissionsService->method('canManageNodeById')->willReturn(true); + $relations = []; + $this->expectNodeInsertion($relations); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $updated = $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Cases', 'targetType' => 'table', 'targetId' => 4], + ['label' => 'Open cases', 'targetType' => 'view', 'targetId' => 6, 'parentIndex' => 0], + ['label' => 'Docs', 'targetType' => 'url', 'url' => 'https://example.org'], + ])); + + $byLabel = []; + foreach ($inserted as $item) { + $byLabel[$item->getLabel()] = $item; + } + $this->assertNull($byLabel['Cases']->getParentId()); + $this->assertSame($byLabel['Cases']->getId(), $byLabel['Open cases']->getParentId()); + $this->assertSame([10, 20, 30], [$byLabel['Cases']->getOrder(), $byLabel['Open cases']->getOrder(), $byLabel['Docs']->getOrder()]); + $this->assertCount(3, $updated->getMenuItems()); + } + + public function testReplaceMenuRefusesAChildUnderAChild(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('top-level'); + $this->service->update(7, 'user-1', null, null, null, null, [ + new MenuItemInput('A', null, null, 'url', null, 'https://a.example', null), + new MenuItemInput('B', null, null, 'url', null, 'https://b.example', null, 0), + new MenuItemInput('C', null, null, 'url', null, 'https://c.example', null, 1), + ]); + } + + /** + * @param list $items + * @return array the label of each item's parent, by label + */ + private static function parentLabels(array $items): array { + $labels = []; + foreach ($items as $item) { + $labels[$item->getId()] = $item->getLabel(); + } + $parents = []; + foreach ($items as $item) { + $parents[$item->getLabel()] = $item->getParentId() === null ? null : $labels[$item->getParentId()]; + } + return $parents; + } + + public function testCreateKeepsChildrenOfASubmittedMenuUnderTheirParent(): void { + $inserted = []; + $this->expectCreation(8, 3, $inserted); + + $this->service->create('Intake portal', 'heart', '', [], 'user-1', 0, MenuItemInput::createListFromInputArray([ + ['label' => 'Docs', 'url' => 'https://example.org/docs'], + ['label' => 'Guide', 'url' => 'https://example.org/guide', 'parentIndex' => 0], + ], allowNewStartPage: true)); + + $this->assertSame(['Docs' => null, 'Guide' => 'Docs'], self::parentLabels($inserted)); + } + + public function testCreateStoresChildrenUnderTheStartPageItem(): void { + $inserted = []; + $this->expectCreation(8, 3, $inserted); + + $this->service->create('Intake portal', 'heart', '', [], 'user-1', 0, MenuItemInput::createListFromInputArray([ + ['label' => 'Overview', 'targetType' => 'page'], + ['label' => 'Docs', 'url' => 'https://example.org/docs', 'parentIndex' => 0], + ], allowNewStartPage: true)); + + $this->assertSame(['Overview' => null, 'Docs' => 'Overview'], self::parentLabels($inserted)); + $this->assertSame([3, null], array_map(static fn (MenuItem $item) => $item->getTargetId(), $inserted)); + } + private function buildPageContent(int $id, int $pageId, int $nodeRelId, int $order): PageContent { $pageContent = new PageContent(); $pageContent->id = $id;