From b1d3f93dc632d66b692b3f9f79138100de4051af Mon Sep 17 00:00:00 2001 From: Remko Date: Mon, 12 Oct 2026 13:35:31 +0200 Subject: [PATCH 1/2] feat(contexts): nest application menu items one level deep A menu item can sit under a top-level item of the same menu. The parent is given as its position in the submitted menu, so a new menu can be saved in one request; parents are stored first so a child can point at its parent's id, while the order keeps the menu's sequence. - an item cannot sit under itself, under a child or under a position the menu does not have; the service keeps its own check for items built without the input validation - menu items expose their parentId in the API Co-authored-by: Ruben van der Linde Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Controller/ContextController.php | 4 +- lib/Db/MenuItem.php | 7 +++- .../Version2400Date20261012030000.php | 40 +++++++++++++++++++ lib/Model/MenuItemInput.php | 27 ++++++++++++- lib/ResponseDefinitions.php | 1 + lib/Service/ContextService.php | 37 ++++++++++++----- openapi.json | 18 ++++++++- src/types/openapi/openapi.ts | 10 ++++- tests/unit/Db/MenuItemTest.php | 1 + tests/unit/Model/MenuItemInputTest.php | 35 ++++++++++++++++ tests/unit/Service/ContextServiceTest.php | 39 ++++++++++++++++++ 11 files changed, 200 insertions(+), 19 deletions(-) create mode 100644 lib/Migration/Version2400Date20261012030000.php diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index 858c1717bb..70876a5fc6 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 + * @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 * * @return DataResponse|DataResponse * diff --git a/lib/Db/MenuItem.php b/lib/Db/MenuItem.php index 606cac32b0..66bfa16f2b 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|null, 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|null, 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->getTechnicalName(), '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 5f58aba545..3c4de6bc36 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(static fn (mixed $item): self => self::createFromInputArray($item), $data); + $items = array_map(static fn (mixed $item): self => self::createFromInputArray($item), $data); + foreach ($items as $position => $item) { + $parentIndex = $item->getParentIndex(); + if ($parentIndex === null) { + continue; + } + if ($parentIndex === $position || !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 391bd93715..d1c074472b 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -236,6 +236,7 @@ * url: string|null, * technicalName: string|null, * order: int, + * parentId: int|null, * broken: bool, * } * diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index bd0ecfca6f..d7c628b1d3 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -171,17 +171,32 @@ protected function replaceMenu(Context $context, array $menuItems, ?string $user $this->menuItemMapper->deleteAllByContextId($context->getId()); $technicalNames = $this->menuTechnicalNames($menuItems); - foreach ($menuItems as $position => $item) { - $menuItem = new MenuItem(); - $menuItem->setContextId($context->getId()); - $menuItem->setLabel($item->getLabel()); - $menuItem->setIcon($item->getIcon()); - $menuItem->setTargetType($item->getTargetType()); - $menuItem->setTargetId($item->getTargetId()); - $menuItem->setUrl($item->getUrl()); - $menuItem->setTechnicalName($technicalNames[$position]); - $menuItem->setOrder(($position + 1) * 10); - $this->menuItemMapper->insert($menuItem); + $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 46f89c28f2..afdf23ea53 100644 --- a/openapi.json +++ b/openapi.json @@ -427,6 +427,7 @@ "url", "technicalName", "order", + "parentId", "broken" ], "properties": { @@ -470,6 +471,11 @@ "type": "integer", "format": "int64" }, + "parentId": { + "type": "integer", + "format": "int64", + "nullable": true + }, "broken": { "type": "boolean" } @@ -12018,7 +12024,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": [ @@ -12049,6 +12055,10 @@ }, "technicalName": { "type": "string" + }, + "parentIndex": { + "type": "integer", + "format": "int64" } } } @@ -12489,7 +12499,7 @@ "type": "array", "nullable": true, "default": null, - "description": "provide this parameter to replace the menu; the views and tables it opens become nodes of the context", + "description": "provide this parameter to replace the menu; 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": [ @@ -12520,6 +12530,10 @@ }, "technicalName": { "type": "string" + }, + "parentIndex": { + "type": "integer", + "format": "int64" } } } diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 0eeb8a1a25..2a3d8e248a 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1202,6 +1202,8 @@ export type components = { readonly technicalName: string | null; /** Format: int64 */ readonly order: number; + /** Format: int64 */ + readonly parentId: number | null; readonly broken: boolean; }; readonly NotifyConfig: { @@ -6898,7 +6900,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 { @@ -6910,6 +6912,8 @@ export interface operations { readonly targetId?: number; readonly url?: string; readonly technicalName?: string; + /** Format: int64 */ + readonly parentIndex?: number; }[] | null; }; }; @@ -7102,7 +7106,7 @@ export interface operations { readonly order: number; } | null; /** - * @description provide this parameter to replace the menu; the views and tables it opens become nodes of the context + * @description provide this parameter to replace the menu; 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 { @@ -7114,6 +7118,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 d4db8ee4d2..947092325c 100644 --- a/tests/unit/Db/MenuItemTest.php +++ b/tests/unit/Db/MenuItemTest.php @@ -27,6 +27,7 @@ public function testJsonSerializeUsesSafeDefaults(): void { 'url' => null, 'technicalName' => null, 'order' => 0, + 'parentId' => null, 'broken' => false, ], $item->jsonSerialize()); } diff --git a/tests/unit/Model/MenuItemInputTest.php b/tests/unit/Model/MenuItemInputTest.php index 49c9e1cd0f..e524d5a6c4 100644 --- a/tests/unit/Model/MenuItemInputTest.php +++ b/tests/unit/Model/MenuItemInputTest.php @@ -60,6 +60,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 d10d323792..ae8aec0c37 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -151,6 +151,45 @@ 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([], []); + $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]]); From 809a459c60721cf8990226d26d51d096b530a88d Mon Sep 17 00:00:00 2001 From: Remko Date: Tue, 13 Oct 2026 13:09:48 +0200 Subject: [PATCH 2/2] refactor(contexts): drop a redundant nesting check and test the parent in the output - an item placed under itself is already refused because its parent then has a parent too, so comparing the parent's position with its own adds nothing - the serialization test now sets a parent and checks that it is returned Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Model/MenuItemInput.php | 4 ++-- tests/unit/Db/MenuItemTest.php | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/Model/MenuItemInput.php b/lib/Model/MenuItemInput.php index b098a0232a..83e93094a1 100644 --- a/lib/Model/MenuItemInput.php +++ b/lib/Model/MenuItemInput.php @@ -102,12 +102,12 @@ public static function createListFromInputArray(mixed $data): array { throw new InvalidArgumentException('The menu items must be a list.'); } $items = array_map(self::createFromInputArray(...), $data); - foreach ($items as $position => $item) { + foreach ($items as $item) { $parentIndex = $item->getParentIndex(); if ($parentIndex === null) { continue; } - if ($parentIndex === $position || !isset($items[$parentIndex]) || $items[$parentIndex]->getParentIndex() !== null) { + 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.'); } } diff --git a/tests/unit/Db/MenuItemTest.php b/tests/unit/Db/MenuItemTest.php index 9316bc7b16..2aafc42725 100644 --- a/tests/unit/Db/MenuItemTest.php +++ b/tests/unit/Db/MenuItemTest.php @@ -42,6 +42,7 @@ public function testJsonSerializeCarriesEveryField(): void { $item->setTargetId(6); $item->setTechnicalName('home'); $item->setOrder(20); + $item->setParentId(4); $json = $item->jsonSerialize(); @@ -51,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 {