diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index ac236b95ae..fe821a9cb9 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -101,7 +101,7 @@ public function show(int $contextId): DataResponse { * @param string $iconName Material design icon name of the context * @param string $description Descriptive text of the context * @psalm-param list $nodes optional nodes to be connected to this context - * @psalm-param ?list $menuItems optional menu of the context, in order; the views and tables it opens become nodes of the context + * @psalm-param ?list $menuItems optional menu of the context, in order; parentIndex is the position of the parent item, for one level of nesting; the views and tables it opens become nodes of the context * * @return DataResponse|DataResponse * @@ -141,7 +141,7 @@ public function create(string $name, string $iconName, string $description = '', * @param ?string $iconName provide this parameter to set a new icon * @param ?string $description provide this parameter to set a new description * @param ?array{id: int, type: int, permissions: int, order: int} $nodes provide this parameter to set a new list of nodes. - * @psalm-param ?list $menuItems provide this parameter to replace the menu; the views and tables it opens become nodes of the context, and an item without a technicalName keeps the one stored for the same target + * @psalm-param ?list $menuItems provide this parameter to replace the menu; 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/Db/MenuItem.php b/lib/Db/MenuItem.php index aca2cafb62..d298dedcd3 100644 --- a/lib/Db/MenuItem.php +++ b/lib/Db/MenuItem.php @@ -28,6 +28,8 @@ * @method setTechnicalName(string $technicalName): void * @method getOrder(): int * @method setOrder(int $order): void + * @method getParentId(): ?int + * @method setParentId(?int $parentId): void * @method isBroken(): bool * @method setBroken(bool $broken): void */ @@ -45,6 +47,7 @@ class MenuItem extends EntitySuper implements JsonSerializable { protected ?string $url = null; protected ?string $technicalName = null; protected ?int $order = null; + protected ?int $parentId = null; // virtual: true when the view or table the item points at no longer exists protected bool $broken = false; @@ -56,11 +59,12 @@ public function __construct() { $this->addType('contextId', 'integer'); $this->addType('targetId', 'integer'); $this->addType('order', 'integer'); + $this->addType('parentId', 'integer'); $this->addType('broken', 'boolean'); } /** - * @return array{id: int, contextId: int, label: string, icon: string|null, targetType: 'view'|'table'|'url', targetId: int|null, url: string|null, technicalName: string, order: int, broken: bool} + * @return array{id: int, contextId: int, label: string, icon: string|null, targetType: 'view'|'table'|'url', targetId: int|null, url: string|null, technicalName: string, order: int, parentId: int|null, broken: bool} */ public function jsonSerialize(): array { return [ @@ -77,6 +81,7 @@ public function jsonSerialize(): array { 'url' => $this->getUrl(), 'technicalName' => $this->technicalName ?? '', 'order' => $this->getOrder() ?? 0, + 'parentId' => $this->getParentId(), 'broken' => $this->broken, ]; } diff --git a/lib/Migration/Version2400Date20261012030000.php b/lib/Migration/Version2400Date20261012030000.php new file mode 100644 index 0000000000..193b824e37 --- /dev/null +++ b/lib/Migration/Version2400Date20261012030000.php @@ -0,0 +1,40 @@ +hasTable('tables_contexts_menu_item')) { + $table = $schema->getTable('tables_contexts_menu_item'); + if (!$table->hasColumn('parent_id')) { + $table->addColumn('parent_id', Types::INTEGER, [ + 'notnull' => false, + 'default' => null, + ]); + } + } + + return $schema; + } +} diff --git a/lib/Model/MenuItemInput.php b/lib/Model/MenuItemInput.php index e8e80906f2..83e93094a1 100644 --- a/lib/Model/MenuItemInput.php +++ b/lib/Model/MenuItemInput.php @@ -30,6 +30,7 @@ public function __construct( private readonly ?int $targetId, private readonly ?string $url, private readonly ?string $technicalName, + private readonly ?int $parentIndex = null, ) { if (trim($this->label) === '' || mb_strlen($this->label) > self::MAX_LABEL_LENGTH) { throw new InvalidArgumentException('A menu item needs a label of at most ' . self::MAX_LABEL_LENGTH . ' characters.'); @@ -74,6 +75,10 @@ public static function createFromInputArray(mixed $data): self { 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']), @@ -82,10 +87,13 @@ public static function createFromInputArray(mixed $data): self { targetId: $isUrl ? null : $targetId, url: $isUrl ? trim((string)($data['url'] ?? '')) : null, technicalName: self::emptyToNull($data['technicalName'] ?? null), + parentIndex: $parentIndex, ); } /** + * A menu with one level of nesting: an item may sit under a top-level item of the same menu. + * * @return list * @throws InvalidArgumentException */ @@ -93,7 +101,17 @@ public static function createListFromInputArray(mixed $data): array { if (!is_array($data) || !array_is_list($data)) { throw new InvalidArgumentException('The menu items must be a list.'); } - return array_map(self::createFromInputArray(...), $data); + $items = array_map(self::createFromInputArray(...), $data); + foreach ($items as $item) { + $parentIndex = $item->getParentIndex(); + if ($parentIndex === null) { + continue; + } + if (!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; } /** @@ -148,4 +166,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/ResponseDefinitions.php b/lib/ResponseDefinitions.php index 166d7feb79..2623695b87 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -238,6 +238,7 @@ * url: string|null, * technicalName: string, * order: int, + * parentId: int|null, * broken: bool, * } * diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 33bc18e9a4..032f30a9c8 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -175,17 +175,32 @@ protected function replaceMenu(Context $context, array $menuItems, ?string $user $this->menuItemMapper->deleteAllByContextId($context->getId()); $technicalNames = $this->menuTechnicalNames($menuItems, $storedMenu); - foreach ($menuItems as $position => $item) { - $menuItem = new MenuItem(); - $menuItem->setContextId($context->getId()); - $menuItem->setLabel($item->getLabel()); - $menuItem->setIcon($item->getIcon()); - $menuItem->setTargetType($item->getTargetType()); - $menuItem->setTargetId($item->getTargetId()); - $menuItem->setUrl($item->getUrl()); - $menuItem->setTechnicalName($technicalNames[$position]); - $menuItem->setOrder(($position + 1) * 10); - $this->menuItemMapper->insert($menuItem); + $topLevelIds = []; + // top-level items first, so a child can point at its parent's id; the order keeps the menu's sequence + 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($context->getId()); + $menuItem->setLabel($item->getLabel()); + $menuItem->setIcon($item->getIcon()); + $menuItem->setTargetType($item->getTargetType()); + $menuItem->setTargetId($item->getTargetId()); + $menuItem->setUrl($item->getUrl()); + $menuItem->setTechnicalName($technicalNames[$position]); + $menuItem->setOrder(($position + 1) * 10); + $menuItem->setParentId($parentIndex === null ? null : $topLevelIds[$parentIndex]); + $storedId = $this->menuItemMapper->insert($menuItem)->getId(); + if ($parentIndex === null) { + $topLevelIds[$position] = $storedId; + } + } } $context->setMenuItems(array_map( diff --git a/openapi.json b/openapi.json index 7d9eb643a1..f6d23972cd 100644 --- a/openapi.json +++ b/openapi.json @@ -427,6 +427,7 @@ "url", "technicalName", "order", + "parentId", "broken" ], "properties": { @@ -469,6 +470,11 @@ "type": "integer", "format": "int64" }, + "parentId": { + "type": "integer", + "format": "int64", + "nullable": true + }, "broken": { "type": "boolean" } @@ -12081,7 +12087,7 @@ "type": "array", "nullable": true, "default": null, - "description": "optional menu of the context, in order; the views and tables it opens become nodes of the context", + "description": "optional menu of the context, in order; parentIndex is the position of the parent item, for one level of nesting; the views and tables it opens become nodes of the context", "items": { "type": "object", "required": [ @@ -12112,6 +12118,10 @@ }, "technicalName": { "type": "string" + }, + "parentIndex": { + "type": "integer", + "format": "int64" } } } @@ -12552,7 +12562,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": [ @@ -12583,6 +12593,10 @@ }, "technicalName": { "type": "string" + }, + "parentIndex": { + "type": "integer", + "format": "int64" } } } diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 933664d25e..671ff1646d 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1202,6 +1202,8 @@ export type components = { readonly technicalName: string; /** Format: int64 */ readonly order: number; + /** Format: int64 */ + readonly parentId: number | null; readonly broken: boolean; }; readonly NotifyConfig: { @@ -6920,7 +6922,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 + * @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 * @default null */ readonly menuItems?: readonly { @@ -6932,6 +6934,8 @@ export interface operations { readonly targetId?: number; readonly url?: string; readonly technicalName?: string; + /** Format: int64 */ + readonly parentIndex?: number; }[] | null; }; }; @@ -7124,7 +7128,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 { @@ -7136,6 +7140,8 @@ export interface operations { readonly targetId?: number; readonly url?: string; readonly technicalName?: string; + /** Format: int64 */ + readonly parentIndex?: number; }[] | null; }; }; diff --git a/tests/unit/Db/MenuItemTest.php b/tests/unit/Db/MenuItemTest.php index e2da546fc9..2aafc42725 100644 --- a/tests/unit/Db/MenuItemTest.php +++ b/tests/unit/Db/MenuItemTest.php @@ -27,6 +27,7 @@ public function testJsonSerializeUsesSafeDefaults(): void { 'url' => null, 'technicalName' => '', 'order' => 0, + 'parentId' => null, 'broken' => false, ], $item->jsonSerialize()); } @@ -41,6 +42,7 @@ public function testJsonSerializeCarriesEveryField(): void { $item->setTargetId(6); $item->setTechnicalName('home'); $item->setOrder(20); + $item->setParentId(4); $json = $item->jsonSerialize(); @@ -50,6 +52,7 @@ public function testJsonSerializeCarriesEveryField(): void { $this->assertSame(6, $json['targetId']); $this->assertSame('home', $json['technicalName']); $this->assertSame(20, $json['order']); + $this->assertSame(4, $json['parentId']); } public static function targetTypeProvider(): array { diff --git a/tests/unit/Model/MenuItemInputTest.php b/tests/unit/Model/MenuItemInputTest.php index fe7a667aac..2881bc58a6 100644 --- a/tests/unit/Model/MenuItemInputTest.php +++ b/tests/unit/Model/MenuItemInputTest.php @@ -62,6 +62,41 @@ public function testRejectsInvalidInput(mixed $input, string $message): void { MenuItemInput::createFromInputArray($input); } + 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 testRejectsAMenuThatIsNotAList(): void { $this->expectException(InvalidArgumentException::class); $this->expectExceptionMessage('must be a list'); diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index 520ac51f7d..cc66f14d64 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -152,6 +152,48 @@ public function testUpdateReplacesTheMenuInOrderAndDerivesTechnicalNamesFromLabe $this->assertSame('Welcome table!', $updated->getMenuItems()[1]['label']); } + 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); + + // built without createListFromInputArray, which already refuses this, to reach the service's own guard + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('top-level'); + $this->service->update(7, 'user-1', null, null, null, null, [ + new MenuItemInput('A', null, 'url', null, 'https://a.example', null), + new MenuItemInput('B', null, 'url', null, 'https://b.example', null, 0), + new MenuItemInput('C', null, 'url', null, 'https://c.example', null, 1), + ]); + } + public function testMenuTargetsBecomeNodesSoRecipientsCanOpenThem(): void { $context = $this->contextWithMenu(7, []); $context->setNodes([101 => ['id' => 101, 'node_id' => 4, 'node_type' => Application::NODE_TYPE_TABLE, 'permissions' => 1]]);