diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index 0489b1ba5b..76780e1609 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -101,7 +101,7 @@ public function show(int $contextId): DataResponse { * @param string $iconName Material design icon name of the context * @param string $description Descriptive text of the context * @psalm-param list $nodes optional nodes to be connected to this context - * @psalm-param ?list $menuItems optional menu of the context, in order; the views and tables it opens become nodes of the context, a page item without a targetId opens the start page; 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, 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 * @@ -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 1041803412..11d05812ab 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 */ @@ -46,6 +48,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, table or page the item points at no longer exists protected bool $broken = false; @@ -57,11 +60,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'|'page', 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'|'page', targetId: int|null, url: string|null, technicalName: string, order: int, parentId: int|null, broken: bool} */ public function jsonSerialize(): array { return [ @@ -79,6 +83,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 c3c0380724..6940c8b4f1 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, private readonly bool $allowNewStartPage = false, ) { if (trim($this->label) === '' || mb_strlen($this->label) > self::MAX_LABEL_LENGTH) { @@ -78,6 +79,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']), @@ -86,11 +91,14 @@ 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. + * * @param bool $allowNewStartPage see createFromInputArray() * @return list * @throws InvalidArgumentException @@ -99,7 +107,17 @@ 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) { + 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; } /** @@ -115,7 +133,7 @@ public function pointsAtNewStartPage(): bool { * @throws InvalidArgumentException */ public function withPageId(int $pageId): self { - return new self($this->label, $this->icon, MenuItem::TARGET_PAGE, $pageId, null, $this->technicalName); + return new self($this->label, $this->icon, MenuItem::TARGET_PAGE, $pageId, null, $this->technicalName, $this->parentIndex); } /** @@ -170,4 +188,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 e75fdbe763..2645185174 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 0a23c8e9bf..975ddb68ec 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -246,17 +246,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 4a70afef1f..22fd8a431e 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" } @@ -12082,7 +12088,7 @@ "type": "array", "nullable": true, "default": null, - "description": "optional menu of the context, in order; the views and tables it opens become nodes of the context, a page item without a targetId opens the start page; 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, 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": [ @@ -12114,6 +12120,10 @@ }, "technicalName": { "type": "string" + }, + "parentIndex": { + "type": "integer", + "format": "int64" } } } @@ -12554,7 +12564,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": [ @@ -12586,6 +12596,10 @@ }, "technicalName": { "type": "string" + }, + "parentIndex": { + "type": "integer", + "format": "int64" } } } diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 0ac2af9067..e4d9d3297d 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, 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, 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 { @@ -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 d20aa9f3a0..b0c09faa2e 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 b0915c2d60..32d66e7946 100644 --- a/tests/unit/Model/MenuItemInputTest.php +++ b/tests/unit/Model/MenuItemInputTest.php @@ -63,6 +63,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 testAPageItemWithoutAnIdOpensTheNewStartPageOnlyWhenAllowed(): void { [$startPage, $page] = MenuItemInput::createListFromInputArray([ ['label' => 'Dashboard', 'targetType' => 'page', 'technicalName' => 'home'], @@ -85,6 +120,17 @@ public function testOnlyAPageItemMayLeaveOutItsIdWhenTheStartPageIsAllowed(): vo MenuItemInput::createFromInputArray(['label' => 'X', 'targetType' => 'view'], allowNewStartPage: true); } + 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 f0c6e761ab..2449d921fa 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -156,6 +156,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]]); @@ -399,6 +441,47 @@ public function testCreateResolvesAPageItemWithoutAnIdToTheStartPage(): void { ], self::describeMenu($inserted)); } + /** + * @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)); + } + public function testAPageOfTheApplicationIsAcceptedAndAddsNoNode(): void { $context = $this->contextWithMenu(7, []); $context->setPages([301 => ['id' => 301, 'page_type' => Page::TYPE_STARTPAGE, 'content' => []]]);