From 403741e324f1430ae8fc0a744b75aa3dc38121ef Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 22 Oct 2026 15:10:23 +0200 Subject: [PATCH 1/4] feat(menu): give applications a menu A menu item has a label, an optional icon and description, and opens a table, a view, a page of the same application or a link. Items have a technical name unique within the application (item_ by default) and may sit under another item. An item whose target is gone is read as broken. Applications carry their menu in their responses, loaded for all applications of a response in one query. Deleting an application deletes its menu, and deletes the application row first inside a retried transaction. Co-authored-by: remko48 Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ruben van der Linde --- lib/Db/Context.php | 11 +- lib/Db/ContextMapper.php | 88 +++---- lib/Db/MenuItem.php | 97 ++++++++ lib/Db/MenuItemMapper.php | 112 +++++++++ .../Version2500Date20261022010000.php | 53 ++++ lib/ResponseDefinitions.php | 16 ++ lib/Service/ContextService.php | 57 ++++- lib/Service/ShareService.php | 23 +- tests/unit/Db/ContextMapperTest.php | 29 ++- tests/unit/Db/ContextTest.php | 28 +++ tests/unit/Db/MenuItemMapperTest.php | 227 ++++++++++++++++++ tests/unit/Db/MenuItemTest.php | 92 +++++++ .../Service/ContextPagesRoundTripTest.php | 4 +- tests/unit/Service/ContextServiceTest.php | 88 ++++++- tests/unit/Service/ShareServiceTest.php | 10 + 15 files changed, 858 insertions(+), 77 deletions(-) create mode 100644 lib/Db/MenuItem.php create mode 100644 lib/Db/MenuItemMapper.php create mode 100644 lib/Migration/Version2500Date20261022010000.php create mode 100644 tests/unit/Db/ContextTest.php create mode 100644 tests/unit/Db/MenuItemMapperTest.php create mode 100644 tests/unit/Db/MenuItemTest.php diff --git a/lib/Db/Context.php b/lib/Db/Context.php index 3fa14f56b5..d465403521 100644 --- a/lib/Db/Context.php +++ b/lib/Db/Context.php @@ -28,6 +28,8 @@ * @method setNodes(array $value): void * @method getPages(): array * @method setPages(array $value): void + * @method getMenuItems(): array + * @method setMenuItems(array $value): void */ class Context extends EntitySuper implements JsonSerializable { protected ?string $name = null; @@ -40,8 +42,9 @@ class Context extends EntitySuper implements JsonSerializable { protected ?array $sharing = null; protected ?array $nodes = null; protected ?array $pages = null; + protected ?array $menuItems = null; - protected const VIRTUAL_PROPERTIES = ['sharing', 'nodes', 'pages']; + protected const VIRTUAL_PROPERTIES = ['sharing', 'nodes', 'pages', 'menuItems']; public function __construct() { $this->addType('id', 'integer'); @@ -56,8 +59,12 @@ public function jsonSerialize(): array { 'iconName' => $this->getIcon(), 'description' => $this->getDescription(), 'owner' => $this->getOwnerId(), - 'ownerType' => $this->getOwnerType() + 'ownerType' => $this->getOwnerType(), ]; + // left out where the menus are not loaded, such as for the navigation bar + if (is_array($this->menuItems)) { + $data['menuItems'] = array_values($this->menuItems); + } // extended data if (is_array($this->sharing) || is_array($this->nodes) || is_array($this->pages)) { diff --git a/lib/Db/ContextMapper.php b/lib/Db/ContextMapper.php index b5dd82380b..181dfb1d07 100644 --- a/lib/Db/ContextMapper.php +++ b/lib/Db/ContextMapper.php @@ -27,6 +27,7 @@ public function __construct( IDBConnection $db, protected UserHelper $userHelper, protected GroupHelper $groupHelper, + protected MenuItemMapper $menuItemMapper, ) { parent::__construct($db, $this->table, Context::class); } @@ -81,7 +82,10 @@ protected function getFindContextBaseQuery(?string $userId): IQueryBuilder { return $qb; } - protected function formatResultRows(array $rows, ?string $userId) { + /** + * @param array>|null $menuItemsByContext the menus by context id, null when they are not loaded + */ + protected function formatResultRows(array $rows, ?string $userId, ?array $menuItemsByContext = null) { $formatted = [ 'id' => $rows[0]['id'], 'name' => $rows[0]['name'], @@ -150,6 +154,13 @@ protected function formatResultRows(array $rows, ?string $userId) { return $carry; }, []); + if ($menuItemsByContext !== null) { + $formatted['menu_items'] = array_map( + static fn (MenuItem $menuItem): array => $menuItem->jsonSerialize(), + $menuItemsByContext[(int)$rows[0]['id']] ?? [], + ); + } + return $this->mapRowToEntity($formatted); } @@ -163,26 +174,9 @@ public function findAll(?string $userId = null): array { $result = $qb->executeQuery(); $r = $this->withOtherPages($result->fetchAllAssociative()); - $contextIds = []; - foreach ($r as $row) { - $contextIds[$row['id']] = 1; - } - $contextIds = array_keys($contextIds); - unset($row); - - $resultEntities = []; - foreach ($contextIds as $contextId) { - $workArray = []; - foreach ($r as $row) { - if ((int)$row['id'] === $contextId) { - $workArray[] = $row; - } - } - $resultEntities[] = $this->formatResultRows($workArray, $userId); - } - - return $resultEntities; + return $this->formatResultSet($r, $userId, true); } + public function findForNavBar(string $userId): array { $qb = $this->getFindContextBaseQuery($userId); $groupIDs = $this->userHelper->getGroupIdsForUser($userId); @@ -213,24 +207,27 @@ public function findForNavBar(string $userId): array { $result = $qb->executeQuery(); $r = $result->fetchAllAssociative(); - $contextIds = []; - foreach ($r as $row) { - $contextIds[$row['id']] = 1; + // the navigation bar shows the applications only, not their menus + return $this->formatResultSet($r, $userId, false); + } + + /** + * Groups the joined rows per context, loading the menus of all contexts in one query. + * + * @return Context[] + * @throws Exception + */ + private function formatResultSet(array $rows, ?string $userId, bool $withMenuItems): array { + $rowsByContext = []; + foreach ($rows as $row) { + $rowsByContext[(int)$row['id']][] = $row; } - $contextIds = array_keys($contextIds); - unset($row); + $menuItemsByContext = $withMenuItems ? $this->menuItemMapper->findByContextIds(array_keys($rowsByContext)) : null; $resultEntities = []; - foreach ($contextIds as $contextId) { - $workArray = []; - foreach ($r as $row) { - if ((int)$row['id'] === $contextId) { - $workArray[] = $row; - } - } - $resultEntities[] = $this->formatResultRows($workArray, $userId); + foreach ($rowsByContext as $contextRows) { + $resultEntities[] = $this->formatResultRows($contextRows, $userId, $menuItemsByContext); } - return $resultEntities; } @@ -249,7 +246,7 @@ public function findById(int $contextId, ?string $userId = null): Context { throw new NotFoundError('Context does not exist'); } - return $this->formatResultRows($r, $userId); + return $this->formatResultRows($r, $userId, $this->menuItemMapper->findByContextIds([$contextId])); } /** @@ -265,25 +262,8 @@ public function findAllContainingNode(int $nodeType, int $nodeId, string $userId $result = $qb->executeQuery(); $r = $result->fetchAllAssociative(); - $contextIds = []; - foreach ($r as $row) { - $contextIds[$row['id']] = 1; - } - $contextIds = array_keys($contextIds); - unset($row); - - $resultEntities = []; - foreach ($contextIds as $contextId) { - $workArray = []; - foreach ($r as $row) { - if ($row['id'] === $contextId) { - $workArray[] = $row; - } - } - $resultEntities[] = $this->formatResultRows($workArray, $userId); - } - - return $resultEntities; + // permission checks read the nodes only, not the menus + return $this->formatResultSet($r, $userId, false); } /** diff --git a/lib/Db/MenuItem.php b/lib/Db/MenuItem.php new file mode 100644 index 0000000000..9866e7af6e --- /dev/null +++ b/lib/Db/MenuItem.php @@ -0,0 +1,97 @@ +addType('id', 'integer'); + $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, description: 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 [ + 'id' => $this->getId(), + 'contextId' => $this->getContextId(), + 'label' => $this->getLabel() ?? '', + 'icon' => $this->getIcon(), + 'description' => $this->getDescription(), + 'targetType' => match ($this->targetType) { + self::TARGET_VIEW => self::TARGET_VIEW, + self::TARGET_TABLE => self::TARGET_TABLE, + self::TARGET_PAGE => self::TARGET_PAGE, + default => self::TARGET_URL, + }, + 'targetId' => $this->getTargetId(), + 'url' => $this->getUrl(), + 'technicalName' => $this->technicalName ?? '', + 'order' => $this->getOrder() ?? 0, + 'parentId' => $this->getParentId(), + 'broken' => $this->broken, + ]; + } +} diff --git a/lib/Db/MenuItemMapper.php b/lib/Db/MenuItemMapper.php new file mode 100644 index 0000000000..618bd0ddf2 --- /dev/null +++ b/lib/Db/MenuItemMapper.php @@ -0,0 +1,112 @@ + */ +class MenuItemMapper extends QBMapper { + use TTransactional; + + private const DB_CHUNK_SIZE = 1_000; + + protected string $table = 'tables_contexts_menu_item'; + + public function __construct(IDBConnection $db) { + parent::__construct($db, $this->table, MenuItem::class); + } + + /** + * Inserts a menu item under the default technical name `item_`. + * + * @throws Exception + */ + public function insertWithDefaultName(MenuItem $menuItem): MenuItem { + return $this->atomic(function () use ($menuItem): MenuItem { + $menuItem = $this->insert($menuItem); + $menuItem->setTechnicalName('item_' . $menuItem->getId()); + return $this->update($menuItem); + }, $this->db); + } + + /** + * @return MenuItem[] ordered by their position in the menu + * @throws Exception + */ + public function findByContextId(int $contextId): array { + return $this->findByContextIds([$contextId])[$contextId] ?? []; + } + + /** + * The menu items of the given contexts, each list ordered by position. An item whose + * view or table no longer exists, or whose page is gone or belongs to another context, + * is marked broken. + * + * @param int[] $contextIds + * @return array> keyed by context id + * @throws Exception + */ + public function findByContextIds(array $contextIds): array { + if ($contextIds === []) { + return []; + } + + $qb = $this->db->getQueryBuilder(); + $brokenExpression = sprintf( + 'CASE WHEN (m.target_type = %s AND v.id IS NULL) OR (m.target_type = %s AND t.id IS NULL) OR (m.target_type = %s AND p.id IS NULL) THEN 1 ELSE 0 END', + $qb->createNamedParameter(MenuItem::TARGET_VIEW), + $qb->createNamedParameter(MenuItem::TARGET_TABLE), + $qb->createNamedParameter(MenuItem::TARGET_PAGE), + ); + $qb->select('m.*') + ->selectAlias($qb->createFunction($brokenExpression), 'broken') + ->from($this->table, 'm') + ->leftJoin('m', 'tables_views', 'v', $qb->expr()->andX( + $qb->expr()->eq('v.id', 'm.target_id'), + $qb->expr()->eq('m.target_type', $qb->createNamedParameter(MenuItem::TARGET_VIEW)), + )) + ->leftJoin('m', 'tables_tables', 't', $qb->expr()->andX( + $qb->expr()->eq('t.id', 'm.target_id'), + $qb->expr()->eq('m.target_type', $qb->createNamedParameter(MenuItem::TARGET_TABLE)), + )) + ->leftJoin('m', 'tables_contexts_page', 'p', $qb->expr()->andX( + $qb->expr()->eq('p.id', 'm.target_id'), + $qb->expr()->eq('m.target_type', $qb->createNamedParameter(MenuItem::TARGET_PAGE)), + $qb->expr()->eq('p.context_id', 'm.context_id'), + )) + ->where($qb->expr()->in('m.context_id', $qb->createParameter('contextIds'))) + ->orderBy('m.context_id', 'ASC') + ->addOrderBy('m.order', 'ASC') + ->addOrderBy('m.id', 'ASC'); + + $byContext = []; + foreach (array_chunk(array_values(array_unique($contextIds)), self::DB_CHUNK_SIZE) as $chunk) { + $qb->setParameter('contextIds', $chunk, IQueryBuilder::PARAM_INT_ARRAY); + foreach ($this->findEntities($qb) as $menuItem) { + $byContext[$menuItem->getContextId()][] = $menuItem; + } + } + return $byContext; + } + + /** + * @throws Exception + */ + public function deleteAllByContextId(int $contextId): void { + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->table) + ->where($qb->expr()->eq('context_id', $qb->createNamedParameter($contextId, IQueryBuilder::PARAM_INT))); + $qb->executeStatement(); + } +} diff --git a/lib/Migration/Version2500Date20261022010000.php b/lib/Migration/Version2500Date20261022010000.php new file mode 100644 index 0000000000..ff118dc93a --- /dev/null +++ b/lib/Migration/Version2500Date20261022010000.php @@ -0,0 +1,53 @@ +hasTable(self::TABLE)) { + return null; + } + + $table = $schema->createTable(self::TABLE); + $table->addColumn('id', Types::INTEGER, ['autoincrement' => true, 'notnull' => true]); + $table->addColumn('context_id', Types::INTEGER, ['notnull' => true]); + $table->addColumn('label', Types::STRING, ['notnull' => true, 'length' => 200]); + $table->addColumn('icon', Types::STRING, ['notnull' => false, 'length' => 64]); + $table->addColumn('description', Types::TEXT, ['notnull' => false]); + $table->addColumn('target_type', Types::STRING, ['notnull' => true, 'length' => 16]); + $table->addColumn('target_id', Types::INTEGER, ['notnull' => false]); + $table->addColumn('url', Types::STRING, ['notnull' => false, 'length' => 2000]); + // Nullable so an item can be inserted first and named `item_` after. + $table->addColumn('technical_name', Types::STRING, ['notnull' => false, 'length' => 200]); + $table->addColumn('order', Types::INTEGER, ['notnull' => true, 'default' => 0]); + $table->addColumn('parent_id', Types::INTEGER, ['notnull' => false]); + $table->setPrimaryKey(['id']); + $table->addUniqueIndex(['context_id', 'technical_name'], 'tables_ctx_menu_name_uq'); + + return $schema; + } +} diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index d750e9a969..82e5cb5eb7 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -234,6 +234,21 @@ * errors_count: int, * } * + * @psalm-type TablesMenuItem = 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, + * } + * * @psalm-type TablesContext = array{ * id: int, * name: string, @@ -241,6 +256,7 @@ * description: string, * owner: string, * ownerType: int, + * menuItems?: list, * } * * @psalm-type TablesWidgetValue = string|int|float|bool|null diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index ed01048c24..780bd5dc0f 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -15,6 +15,8 @@ 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; use OCA\Tables\Db\PageContentMapper; @@ -66,6 +68,7 @@ public function __construct( private ViewMapper $viewMapper, private WidgetService $widgetService, private GridWidgetTypes $gridWidgetTypes, + private MenuItemMapper $menuItemMapper, ) { } @@ -85,7 +88,11 @@ public function findAll(?string $userId): array { $this->logger->warning($error); throw new InternalError($error); } - return $this->contextMapper->findAll($userId); + $contexts = $this->contextMapper->findAll($userId); + foreach ($contexts as $context) { + $this->markMenuForReader($context, $userId); + } + return $contexts; } public function findForNavigation(string $userId): array { @@ -130,7 +137,46 @@ public function findById(int $id, ?string $userId): Context { throw new InternalError($error); } - return $this->contextMapper->findById($id, $userId); + $context = $this->contextMapper->findById($id, $userId); + $this->markMenuForReader($context, $userId); + return $context; + } + + /** + * Marks a menu item as broken for a reader who cannot open its table or view, the + * way an item whose target is gone is broken for everyone. A table or view of the + * application itself is open to everyone the application is shared with. + */ + private function markMenuForReader(Context $context, ?string $userId): void { + $menuItems = $context->getMenuItems(); + if ($userId === null || !is_array($menuItems)) { + return; + } + $readableNodes = []; + foreach ($context->getNodes() ?? [] as $node) { + if (((int)$node['permissions'] & Application::PERMISSION_READ) !== 0) { + $readableNodes[sprintf('t%di%d', $node['node_type'], $node['node_id'])] = true; + } + } + $changed = false; + foreach ($menuItems as $index => $item) { + $nodeType = match ($item['targetType'] ?? null) { + MenuItem::TARGET_VIEW => Application::NODE_TYPE_VIEW, + MenuItem::TARGET_TABLE => Application::NODE_TYPE_TABLE, + default => null, + }; + $nodeId = $item['targetId'] ?? null; + if ($nodeType === null || !is_int($nodeId) || ($item['broken'] ?? false) || isset($readableNodes[sprintf('t%di%d', $nodeType, $nodeId)])) { + continue; + } + if (!$this->permissionsService->canAccessNodeById($nodeType, $nodeId, $userId)) { + $menuItems[$index]['broken'] = true; + $changed = true; + } + } + if ($changed) { + $context->setMenuItems($menuItems); + } } /** @@ -341,8 +387,12 @@ public function update(int $contextId, string $userId, ?string $name, ?string $i public function delete(int $contextId, string $userId): Context { $context = $this->contextMapper->findById($contextId, $userId); - $this->atomic(function () use ($context): void { + // Deleting the application first takes the write lock before anything is read, so on + // SQLite a concurrent write fails this transaction at once and the retry runs it again. + $this->atomicRetry(function () use ($context): void { + $this->contextMapper->delete($context); $this->shareService->deleteAllForContext($context); + $this->menuItemMapper->deleteAllByContextId($context->getId()); $this->contextNodeRelMapper->deleteAllByContextId($context->getId()); $pageIds = $this->pageMapper->getPageIdsForContext($context->getId()); $this->widgetService->deleteByPageIds($pageIds); @@ -350,7 +400,6 @@ public function delete(int $contextId, string $userId): Context { $this->pageContentMapper->deleteByPageId($pageId); $this->pageMapper->deleteByPageId($pageId); } - $this->contextMapper->delete($context); }, $this->dbc); $this->auditLogService->log(Operation::ContextDeleted, 'Tables application with ID %s was deleted', [ 'contextId' => $context->getId(), diff --git a/lib/Service/ShareService.php b/lib/Service/ShareService.php index 1d5408c188..7bbbd7f250 100644 --- a/lib/Service/ShareService.php +++ b/lib/Service/ShareService.php @@ -865,18 +865,19 @@ public function deleteAllForView(View $view):void { } } + /** + * A database error is passed on, so a transaction around the call fails as a whole. + * + * @throws Exception + */ public function deleteAllForContext(Context $context): void { - try { - $this->atomic(function () use ($context): void { - $shares = $this->mapper->findAllSharesForNode('context', $context->getId(), $this->userId); - foreach ($shares as $share) { - $this->contextNavigationMapper->deleteByShareId($share->getId()); - } - $this->mapper->deleteByNode($context->getId(), 'context'); - }, $this->dbc); - } catch (Exception) { - $this->logger->error('something went wrong while deleting shares for context: ' . $context->getId()); - } + $this->atomic(function () use ($context): void { + $shares = $this->mapper->findAllSharesForNode('context', $context->getId(), $this->userId); + foreach ($shares as $share) { + $this->contextNavigationMapper->deleteByShareId($share->getId()); + } + $this->mapper->deleteByNode($context->getId(), 'context'); + }, $this->dbc); } /** diff --git a/tests/unit/Db/ContextMapperTest.php b/tests/unit/Db/ContextMapperTest.php index 4ac495ad35..4f524d7553 100644 --- a/tests/unit/Db/ContextMapperTest.php +++ b/tests/unit/Db/ContextMapperTest.php @@ -14,6 +14,8 @@ 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; use OCA\Tables\Db\PageContentMapper; @@ -31,6 +33,7 @@ class ContextMapperTest extends DatabaseTestCase { private ContextMapper $mapper; private ContextNodeRelationMapper $nodeRelMapper; private PageMapper $pageMapper; + private MenuItemMapper $menuItemMapper; private PageContentMapper $pageContentMapper; /** @var Context[] */ private array $contexts = []; @@ -39,7 +42,8 @@ protected function setUp(): void { parent::setUp(); $userHelper = $this->createMock(UserHelper::class); $userHelper->method('getGroupIdsForUser')->willReturn([]); - $this->mapper = new ContextMapper($this->connectionAdapter, $userHelper, $this->createMock(GroupHelper::class)); + $this->menuItemMapper = new MenuItemMapper($this->connectionAdapter); + $this->mapper = new ContextMapper($this->connectionAdapter, $userHelper, $this->createMock(GroupHelper::class), $this->menuItemMapper); $this->nodeRelMapper = new ContextNodeRelationMapper($this->connectionAdapter); $this->pageMapper = new PageMapper($this->connectionAdapter); $this->pageContentMapper = new PageContentMapper($this->connectionAdapter); @@ -52,6 +56,7 @@ protected function tearDown(): void { $this->pageMapper->deleteByPageId($pageId); } $this->nodeRelMapper->deleteAllByContextId($context->getId()); + $this->menuItemMapper->deleteAllByContextId($context->getId()); $this->mapper->delete($context); } parent::tearDown(); @@ -86,6 +91,28 @@ public function testGridPageHasItsFieldsAndNoStartPageContent(): void { $this->assertSame([$first->getId(), $second->getId()], array_keys($this->mapper->findById($context->getId(), self::OWNER)->getNodes())); } + public function testTheApplicationsContainingANodeAreFoundWithoutTheirMenus(): void { + $context = $this->createContext(); + $this->addNode($context, 11); + $other = $this->createContext(); + $this->addNode($other, 12); + $menuItem = new MenuItem(); + $menuItem->setContextId($context->getId()); + $menuItem->setLabel('Home'); + $menuItem->setTargetType(MenuItem::TARGET_URL); + $menuItem->setUrl('https://example.com'); + $menuItem->setTechnicalName('home'); + $this->menuItemMapper->insert($menuItem); + + $found = $this->mapper->findAllContainingNode(Application::NODE_TYPE_TABLE, 11, self::OWNER); + + $this->assertSame([$context->getId()], array_map(static fn (Context $context): int => $context->getId(), $found)); + $this->assertSame([11], array_column($found[0]->getNodes(), 'node_id')); + $this->assertNull($found[0]->getMenuItems()); + $this->assertArrayNotHasKey('menuItems', $found[0]->jsonSerialize()); + $this->assertSame(['home'], array_column($this->mapper->findById($context->getId(), self::OWNER)->getMenuItems(), 'technicalName')); + } + private function createContext(): Context { $context = new Context(); $context->setName('Mapper test'); diff --git a/tests/unit/Db/ContextTest.php b/tests/unit/Db/ContextTest.php new file mode 100644 index 0000000000..3b6b44af4d --- /dev/null +++ b/tests/unit/Db/ContextTest.php @@ -0,0 +1,28 @@ +assertArrayNotHasKey('menuItems', (new Context())->jsonSerialize()); + } + + public function testALoadedMenuIsCarriedEvenWhenEmpty(): void { + $context = new Context(); + $context->setMenuItems([]); + $this->assertSame([], $context->jsonSerialize()['menuItems']); + + $context->setMenuItems([3 => ['id' => 3]]); + $this->assertSame([['id' => 3]], $context->jsonSerialize()['menuItems']); + } +} diff --git a/tests/unit/Db/MenuItemMapperTest.php b/tests/unit/Db/MenuItemMapperTest.php new file mode 100644 index 0000000000..f8286eab77 --- /dev/null +++ b/tests/unit/Db/MenuItemMapperTest.php @@ -0,0 +1,227 @@ +cleanupTablesData(); + $this->menuItemMapper = new MenuItemMapper($this->connectionAdapter); + $this->contextMapper = new ContextMapper( + $this->connectionAdapter, + $this->createMock(UserHelper::class), + $this->createMock(GroupHelper::class), + $this->menuItemMapper, + ); + } + + public function testItemsWhoseViewOrTableIsGoneAreBroken(): void { + $contextId = $this->createContext('Intake'); + $tableId = $this->createTestTable()['id']; + $viewId = $this->createView($tableId); + $this->createMenuItem($contextId, 'live_view', MenuItem::TARGET_VIEW, $viewId); + $this->createMenuItem($contextId, 'gone_view', MenuItem::TARGET_VIEW, $viewId + 100); + $this->createMenuItem($contextId, 'live_table', MenuItem::TARGET_TABLE, $tableId); + $this->createMenuItem($contextId, 'gone_table', MenuItem::TARGET_TABLE, $tableId + 100); + // A link points at neither, so its target id never makes it broken + $this->createMenuItem($contextId, 'link', MenuItem::TARGET_URL, $viewId + 100); + + $broken = []; + foreach ($this->menuItemMapper->findByContextId($contextId) as $menuItem) { + $broken[$menuItem->getTechnicalName()] = $menuItem->isBroken(); + } + + $this->assertSame([ + 'live_view' => false, + 'gone_view' => true, + 'live_table' => false, + 'gone_table' => true, + 'link' => false, + ], $broken); + } + + public function testItemsWhosePageIsGoneOrForeignAreBroken(): void { + $contextId = $this->createContext('Intake'); + $otherContextId = $this->createContext('Reports'); + $pageId = $this->createPage($contextId); + $foreignPageId = $this->createPage($otherContextId); + $this->createMenuItem($contextId, 'live_page', MenuItem::TARGET_PAGE, $pageId); + $this->createMenuItem($contextId, 'gone_page', MenuItem::TARGET_PAGE, $foreignPageId + 100); + $this->createMenuItem($contextId, 'foreign_page', MenuItem::TARGET_PAGE, $foreignPageId); + // A view with the id of a live page is still judged by the views table + $this->createMenuItem($contextId, 'view_with_page_id', MenuItem::TARGET_VIEW, $pageId); + + $broken = []; + foreach ($this->menuItemMapper->findByContextId($contextId) as $menuItem) { + $broken[$menuItem->getTechnicalName()] = $menuItem->isBroken(); + } + + $this->assertSame([ + 'live_page' => false, + 'gone_page' => true, + 'foreign_page' => true, + 'view_with_page_id' => true, + ], $broken); + } + + public function testItemsAreOrderedAndGroupedPerApplication(): void { + $intakeId = $this->createContext('Intake'); + $reportsId = $this->createContext('Reports'); + $this->createMenuItem($intakeId, 'second', MenuItem::TARGET_URL, null, 20); + $this->createMenuItem($reportsId, 'overview', MenuItem::TARGET_URL, null, 0); + $this->createMenuItem($intakeId, 'first', MenuItem::TARGET_URL, null, 10); + + $menus = $this->menuItemMapper->findByContextIds([$intakeId, $reportsId]); + + $this->assertSame([ + $intakeId => ['first', 'second'], + $reportsId => ['overview'], + ], array_map(self::technicalNames(...), $menus)); + $this->assertSame([], $this->menuItemMapper->findByContextIds([])); + } + + public function testFindByIdIncludesTheMenu(): void { + $contextId = $this->createContext('Intake'); + $this->createMenuItem($contextId, 'home', MenuItem::TARGET_URL, null); + + $menuItems = $this->contextMapper->findById($contextId)->getMenuItems(); + + $this->assertSame(['home'], array_column($menuItems, 'technicalName')); + } + + public function testFindAllIncludesTheMenus(): void { + $intakeId = $this->createContext('Intake'); + $reportsId = $this->createContext('Reports'); + $this->createMenuItem($intakeId, 'home', MenuItem::TARGET_URL, null); + $this->createMenuItem($reportsId, 'overview', MenuItem::TARGET_URL, null); + + $menus = []; + foreach ($this->contextMapper->findAll() as $context) { + $menus[$context->getId()] = array_column($context->getMenuItems(), 'technicalName'); + } + ksort($menus); + + $this->assertSame([$intakeId => ['home'], $reportsId => ['overview']], $menus); + } + + public function testATechnicalNameIsUniqueWithinAnApplication(): void { + $intakeId = $this->createContext('Intake'); + $reportsId = $this->createContext('Reports'); + $this->createMenuItem($intakeId, 'home', MenuItem::TARGET_URL, null); + $this->createMenuItem($reportsId, 'home', MenuItem::TARGET_URL, null); + + try { + $this->createMenuItem($intakeId, 'home', MenuItem::TARGET_URL, null); + $this->fail('The unique index accepted a second item named home'); + } catch (Exception $e) { + $this->assertSame(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION, $e->getReason()); + } + } + + public function testAnItemWithoutAGivenNameIsNamedByItsId(): void { + $intakeId = $this->createContext('Intake'); + $menuItem = new MenuItem(); + $menuItem->setContextId($intakeId); + $menuItem->setLabel('Docs'); + $menuItem->setTargetType(MenuItem::TARGET_URL); + $menuItem->setUrl('https://example.org'); + $menuItem = $this->menuItemMapper->insertWithDefaultName($menuItem); + + $this->assertSame('item_' . $menuItem->getId(), $menuItem->getTechnicalName()); + $this->assertSame(['item_' . $menuItem->getId()], self::technicalNames($this->menuItemMapper->findByContextId($intakeId))); + } + + public function testOnlyTheMenuOfTheGivenApplicationIsDeleted(): void { + $intakeId = $this->createContext('Intake'); + $reportsId = $this->createContext('Reports'); + $this->createMenuItem($intakeId, 'home', MenuItem::TARGET_URL, null); + $this->createMenuItem($reportsId, 'overview', MenuItem::TARGET_URL, null); + + $this->menuItemMapper->deleteAllByContextId($intakeId); + + $this->assertSame([], $this->menuItemMapper->findByContextId($intakeId)); + $this->assertSame(['overview'], self::technicalNames($this->menuItemMapper->findByContextId($reportsId))); + } + + /** + * @param list $menuItems + * @return list + */ + private static function technicalNames(array $menuItems): array { + return array_map(static fn (MenuItem $menuItem): string => $menuItem->getTechnicalName(), $menuItems); + } + + private function createContext(string $name): int { + $qb = $this->connection->getQueryBuilder(); + $qb->insert('tables_contexts_context') + ->values([ + 'name' => $qb->createNamedParameter($name), + 'icon' => $qb->createNamedParameter('star'), + 'description' => $qb->createNamedParameter(''), + 'owner_id' => $qb->createNamedParameter('user1'), + 'owner_type' => $qb->createNamedParameter(0), + ]) + ->executeStatement(); + return (int)$this->connection->lastInsertId(); + } + + private function createPage(int $contextId): int { + $qb = $this->connection->getQueryBuilder(); + $qb->insert('tables_contexts_page') + ->values([ + 'context_id' => $qb->createNamedParameter($contextId), + 'page_type' => $qb->createNamedParameter('startpage'), + 'technical_name' => $qb->createNamedParameter('dashboard'), + ]) + ->executeStatement(); + return (int)$this->connection->lastInsertId(); + } + + private function createView(int $tableId): int { + $qb = $this->connection->getQueryBuilder(); + $qb->insert('tables_views') + ->values([ + 'table_id' => $qb->createNamedParameter($tableId), + 'title' => $qb->createNamedParameter('Open requests'), + 'description' => $qb->createNamedParameter(''), + 'created_by' => $qb->createNamedParameter('user1'), + 'created_at' => $qb->createNamedParameter(date('Y-m-d H:i:s')), + 'last_edit_by' => $qb->createNamedParameter('user1'), + 'last_edit_at' => $qb->createNamedParameter(date('Y-m-d H:i:s')), + ]) + ->executeStatement(); + return (int)$this->connection->lastInsertId(); + } + + private function createMenuItem(int $contextId, string $technicalName, string $targetType, ?int $targetId, int $order = 0): void { + $menuItem = new MenuItem(); + $menuItem->setContextId($contextId); + $menuItem->setLabel($technicalName); + $menuItem->setTargetType($targetType); + $menuItem->setTargetId($targetId); + $menuItem->setTechnicalName($technicalName); + $menuItem->setOrder($order); + $this->menuItemMapper->insert($menuItem); + } +} diff --git a/tests/unit/Db/MenuItemTest.php b/tests/unit/Db/MenuItemTest.php new file mode 100644 index 0000000000..2bf5ba417b --- /dev/null +++ b/tests/unit/Db/MenuItemTest.php @@ -0,0 +1,92 @@ +setContextId(3); + + $this->assertSame([ + 'id' => null, + 'contextId' => 3, + 'label' => '', + 'icon' => null, + 'description' => null, + 'targetType' => MenuItem::TARGET_URL, + 'targetId' => null, + 'url' => null, + 'technicalName' => '', + 'order' => 0, + 'parentId' => null, + 'broken' => false, + ], $item->jsonSerialize()); + } + + public function testJsonSerializeCarriesEveryField(): void { + $item = new MenuItem(); + $item->setId(9); + $item->setContextId(3); + $item->setLabel('Intake home'); + $item->setIcon('home'); + $item->setDescription('Where intake starts'); + $item->setTargetType(MenuItem::TARGET_VIEW); + $item->setTargetId(6); + $item->setTechnicalName('home'); + $item->setOrder(20); + $item->setParentId(4); + + $json = $item->jsonSerialize(); + + $this->assertSame(9, $json['id']); + $this->assertSame('Intake home', $json['label']); + $this->assertSame('home', $json['icon']); + $this->assertSame('Where intake starts', $json['description']); + $this->assertSame('view', $json['targetType']); + $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 { + return [ + 'view' => [MenuItem::TARGET_VIEW], + 'table' => [MenuItem::TARGET_TABLE], + 'url' => [MenuItem::TARGET_URL], + 'page' => [MenuItem::TARGET_PAGE], + ]; + } + + /** + * @dataProvider targetTypeProvider + */ + public function testJsonSerializeKeepsEachTargetType(string $targetType): void { + $item = new MenuItem(); + $item->setTargetType($targetType); + + $this->assertSame($targetType, $item->jsonSerialize()['targetType']); + } + + public function testBrokenIsReportedButNeverStored(): void { + $item = new MenuItem(); + $item->setBroken(true); + + $this->assertTrue($item->jsonSerialize()['broken']); + $this->assertArrayNotHasKey('broken', $item->getUpdatedFields()); + } + + public function testTargetTypesAreTheFourKnownOnes(): void { + $this->assertSame(['view', 'table', 'url', 'page'], MenuItem::TARGET_TYPES); + } +} diff --git a/tests/unit/Service/ContextPagesRoundTripTest.php b/tests/unit/Service/ContextPagesRoundTripTest.php index b2951a5578..53dafa9f2e 100644 --- a/tests/unit/Service/ContextPagesRoundTripTest.php +++ b/tests/unit/Service/ContextPagesRoundTripTest.php @@ -14,6 +14,7 @@ use OCA\Tables\Db\ContextMapper; use OCA\Tables\Db\ContextNodeRelation; use OCA\Tables\Db\ContextNodeRelationMapper; +use OCA\Tables\Db\MenuItemMapper; use OCA\Tables\Db\Page; use OCA\Tables\Db\PageContentMapper; use OCA\Tables\Db\PageMapper; @@ -58,7 +59,7 @@ protected function setUp(): void { $l10n->method('t')->willReturnArgument(0); $userHelper = $this->createMock(UserHelper::class); $userHelper->method('getGroupIdsForUser')->willReturn([]); - $this->contextMapper = new ContextMapper($this->connectionAdapter, $userHelper, $this->createMock(GroupHelper::class)); + $this->contextMapper = new ContextMapper($this->connectionAdapter, $userHelper, $this->createMock(GroupHelper::class), new MenuItemMapper($this->connectionAdapter)); $this->nodeRelMapper = new ContextNodeRelationMapper($this->connectionAdapter); $this->pageMapper = new PageMapper($this->connectionAdapter); $this->widgetService = new WidgetService( @@ -84,6 +85,7 @@ protected function setUp(): void { $this->createMock(ViewMapper::class), $this->widgetService, new GridWidgetTypes($l10n), + new MenuItemMapper($this->connectionAdapter), ); } diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index 9dd2b6f031..1aef07dd6a 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -15,6 +15,7 @@ use OCA\Tables\Db\ContextMapper; use OCA\Tables\Db\ContextNodeRelation; use OCA\Tables\Db\ContextNodeRelationMapper; +use OCA\Tables\Db\MenuItemMapper; use OCA\Tables\Db\Page; use OCA\Tables\Db\PageContent; use OCA\Tables\Db\PageContentMapper; @@ -41,6 +42,9 @@ final class ContextServiceTest extends TestCase { private MockObject $userManager; private MockObject $auditLogService; private MockObject $widgetService; + private MockObject $menuItemMapper; + private MockObject $permissionsService; + private MockObject $shareService; private MockObject $dbConnection; protected function setUp(): void { @@ -51,14 +55,15 @@ protected function setUp(): void { $this->pageMapper = $this->createMock(PageMapper::class); $this->pageContentMapper = $this->createMock(PageContentMapper::class); $logger = $this->createMock(LoggerInterface::class); - $permissionsService = $this->createMock(PermissionsService::class); + $this->permissionsService = $this->createMock(PermissionsService::class); $this->userManager = $this->createMock(IUserManager::class); $this->auditLogService = $this->createMock(AuditLogServiceInterface::class); $this->dbConnection = $this->createMock(IDBConnection::class); $this->widgetService = $this->createMock(WidgetService::class); + $this->menuItemMapper = $this->createMock(MenuItemMapper::class); $l10n = $this->createMock(IL10N::class); $l10n->method('t')->willReturnArgument(0); - $shareService = $this->createMock(ShareService::class); + $this->shareService = $this->createMock(ShareService::class); $navigationManager = $this->createMock(INavigationManager::class); $urlGenerator = $this->createMock(IURLGenerator::class); @@ -68,11 +73,11 @@ protected function setUp(): void { $this->pageMapper, $this->pageContentMapper, $logger, - $permissionsService, + $this->permissionsService, $this->userManager, $this->auditLogService, $this->dbConnection, - $shareService, + $this->shareService, false, $navigationManager, $urlGenerator, @@ -80,6 +85,7 @@ protected function setUp(): void { $this->createMock(ViewMapper::class), $this->widgetService, new GridWidgetTypes($l10n), + $this->menuItemMapper, ); } @@ -305,6 +311,35 @@ public function testDeleteRemovesTheWidgetsOfEveryPage(): void { $this->service->delete(7, 'user-1'); } + public function testDeleteRemovesTheApplicationBeforeWhatBelongsToIt(): void { + $context = new Context(); + $context->id = 7; + $this->contextMapper->method('findById')->willReturn($context); + $this->pageMapper->method('getPageIdsForContext')->willReturn([301]); + $calls = []; + $this->contextMapper->method('delete')->willReturnCallback(static function (Context $deleted) use (&$calls): Context { + $calls[] = 'application'; + return $deleted; + }); + $this->menuItemMapper->method('deleteAllByContextId')->with(7)->willReturnCallback(static function () use (&$calls): void { + $calls[] = 'menu'; + }); + $this->contextNodeRelMapper->method('deleteAllByContextId')->willReturnCallback(static function () use (&$calls): void { + $calls[] = 'nodes'; + }); + $this->widgetService->method('deleteByPageIds')->willReturnCallback(static function () use (&$calls): void { + $calls[] = 'widgets'; + }); + $this->pageMapper->method('deleteByPageId')->willReturnCallback(static function () use (&$calls): int { + $calls[] = 'page'; + return 1; + }); + + $this->service->delete(7, 'alice'); + + $this->assertSame(['application', 'menu', 'nodes', 'widgets', 'page'], $calls); + } + public function testDeletingANodeUnlinksItsWidgets(): void { $this->contextNodeRelMapper->method('getRelIdsForNode')->with(11, 0)->willReturn([101, 102]); @@ -597,4 +632,49 @@ public function testAnImportedPageKeepsAtMostTheMaximumNumberOfWidgets(): void { self::assertCount(GridWidgetTypes::MAX_WIDGETS_PER_PAGE, $record['widgets']); } + + public function testAMenuItemIsBrokenForAReaderWhoCannotOpenItsTarget(): void { + $context = new Context(); + $context->id = 7; + $context->setNodes([101 => ['id' => 101, 'node_id' => 11, 'node_type' => 0, 'permissions' => 1]]); + $context->setMenuItems([ + ['id' => 1, 'targetType' => 'table', 'targetId' => 11, 'broken' => false], + ['id' => 2, 'targetType' => 'table', 'targetId' => 5, 'broken' => false], + ['id' => 3, 'targetType' => 'view', 'targetId' => 6, 'broken' => false], + ['id' => 4, 'targetType' => 'url', 'targetId' => null, 'broken' => false], + ['id' => 5, 'targetType' => 'table', 'targetId' => 9, 'broken' => true], + ]); + $this->contextMapper->method('findById')->willReturnCallback(static fn (): Context => clone $context); + $this->contextMapper->method('findAll')->willReturnCallback(static fn (): array => [clone $context]); + $this->permissionsService->method('canAccessNodeById')->willReturnCallback( + static fn (int $nodeType, int $nodeId, string $userId): bool => $userId === 'owner' || ($nodeType === 1 && $nodeId === 6), + ); + + self::assertSame([false, false, false, false, true], array_column($this->service->findById(7, 'owner')->getMenuItems(), 'broken')); + self::assertSame([false, true, false, false, true], array_column($this->service->findById(7, 'sharee')->getMenuItems(), 'broken')); + self::assertSame([false, true, false, false, true], array_column($this->service->findAll('sharee')[0]->getMenuItems(), 'broken')); + } + + public function testDeletingAnApplicationRetriesATransientDatabaseError(): void { + $context = new Context(); + $context->id = 7; + $this->contextMapper->method('findById')->willReturn($context); + $this->pageMapper->method('getPageIdsForContext')->willReturn([]); + $deadlock = \OC\DB\Exceptions\DbalException::wrap(new \Doctrine\DBAL\Exception\DeadlockException( + \Doctrine\DBAL\Driver\PDO\Exception::new(new \PDOException('deadlock')), + null, + )); + $attempts = 0; + $this->shareService->method('deleteAllForContext')->willReturnCallback(static function () use (&$attempts, $deadlock): void { + if (++$attempts === 1) { + throw $deadlock; + } + }); + $this->contextMapper->expects($this->exactly(2))->method('delete'); + $this->dbConnection->expects($this->once())->method('rollBack'); + + $this->service->delete(7, 'alice'); + + self::assertSame(2, $attempts); + } } diff --git a/tests/unit/Service/ShareServiceTest.php b/tests/unit/Service/ShareServiceTest.php index 1ab87968e5..a09f066b5c 100644 --- a/tests/unit/Service/ShareServiceTest.php +++ b/tests/unit/Service/ShareServiceTest.php @@ -351,4 +351,14 @@ public function testDeleteForShareReviewPropagatesDbException(): void { $this->expectException(Exception::class); $this->shareService->deleteForShareReview(5); } + + public function testDeletingTheSharesOfAContextPassesADatabaseErrorOn(): void { + $context = new \OCA\Tables\Db\Context(); + $context->id = 7; + $this->mapper->method('findAllSharesForNode')->willReturn([]); + $this->mapper->method('deleteByNode')->willThrowException(new Exception('locked')); + + $this->expectException(Exception::class); + $this->shareService->deleteAllForContext($context); + } } From 7d89fa5b11e1f17cbdb2d8c8e505134ac80ba539 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 22 Oct 2026 15:10:23 +0200 Subject: [PATCH 2/4] 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 | 77 ++++++++++++++++++++++++++++++++++++ src/types/openapi/openapi.ts | 21 ++++++++++ 2 files changed, 98 insertions(+) diff --git a/openapi.json b/openapi.json index b0e4a2c6cd..8337c1cc33 100644 --- a/openapi.json +++ b/openapi.json @@ -313,6 +313,12 @@ "ownerType": { "type": "integer", "format": "int64" + }, + "menuItems": { + "type": "array", + "items": { + "$ref": "#/components/schemas/MenuItem" + } } } }, @@ -420,6 +426,77 @@ } } }, + "MenuItem": { + "type": "object", + "required": [ + "id", + "contextId", + "label", + "icon", + "description", + "targetType", + "targetId", + "url", + "technicalName", + "order", + "parentId", + "broken" + ], + "properties": { + "id": { + "type": "integer", + "format": "int64" + }, + "contextId": { + "type": "integer", + "format": "int64" + }, + "label": { + "type": "string" + }, + "icon": { + "type": "string", + "nullable": true + }, + "description": { + "type": "string", + "nullable": true + }, + "targetType": { + "type": "string", + "enum": [ + "view", + "table", + "url", + "page" + ] + }, + "targetId": { + "type": "integer", + "format": "int64", + "nullable": true + }, + "url": { + "type": "string", + "nullable": true + }, + "technicalName": { + "type": "string" + }, + "order": { + "type": "integer", + "format": "int64" + }, + "parentId": { + "type": "integer", + "format": "int64", + "nullable": true + }, + "broken": { + "type": "boolean" + } + } + }, "NotifyConfig": { "type": "object", "required": [ diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 301ff9a878..ab6ba6e74f 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1245,6 +1245,7 @@ export type components = { readonly owner: string; /** Format: int64 */ readonly ownerType: number; + readonly menuItems?: readonly components["schemas"]["MenuItem"][]; }; readonly ContextNavigation: { /** Format: int64 */ @@ -1279,6 +1280,26 @@ export type components = { readonly shareToken: string; readonly url: string; }; + readonly MenuItem: { + /** Format: int64 */ + readonly id: number; + /** Format: int64 */ + readonly contextId: number; + readonly label: string; + readonly icon: string | null; + readonly description: string | null; + /** @enum {string} */ + readonly targetType: "view" | "table" | "url" | "page"; + /** Format: int64 */ + readonly targetId: number | null; + readonly url: string | null; + readonly technicalName: string; + /** Format: int64 */ + readonly order: number; + /** Format: int64 */ + readonly parentId: number | null; + readonly broken: boolean; + }; readonly NotifyConfig: { readonly "notify-assigned": boolean; readonly "notify-column": boolean; From 63ff59c3567b6fcaaabb0a04e047a3ab7c4ae296 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 22 Oct 2026 15:10:24 +0200 Subject: [PATCH 3/4] 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 | 355 +++++++++++- .../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 | 525 +++++++++++++++++- 12 files changed, 1409 insertions(+), 20 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 780bd5dc0f..e4f6a8b4e6 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -32,6 +32,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\Service\Support\AuditLogServiceInterface; @@ -41,6 +42,7 @@ use OCP\AppFramework\Db\TTransactional; use OCP\DB\Exception; 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,6 +207,14 @@ 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->auditLogService->log(Operation::ContextCreated, 'Tables application with ID %s was created for user %s', [ 'contextId' => $context->getId(), @@ -206,17 +222,331 @@ public function create(string $name, string $iconName, string $description, arra 'nodes' => array_map(static fn (array $node): array => ['nodeType' => (int)$node['type'], 'nodeId' => (int)$node['id']], $nodes), ]); + $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. + $changes = []; + $context = $this->atomicRetry( + function () use ($contextId, $userId, $name, $iconName, $description, $nodes, $menuItems, &$changes): Context { + return $this->applyUpdate($contextId, $userId, $name, $iconName, $description, $nodes, $menuItems, $changes); + }, + $this->dbc, + ); + $this->auditLogService->log(Operation::ContextUpdated, 'Tables application with ID %s was updated', ['contextId' => $contextId] + $changes); + return $context; + } + + /** + * @psalm-param list $nodes + * @throws Exception + * @throws DoesNotExistException + * @throws PermissionError|MultipleObjectsReturnedException + * @param list|null $menuItems + * @param array>> $changes set to the nodes added, removed and updated + */ + private function applyUpdate(int $contextId, string $userId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $menuItems, array &$changes): 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)); @@ -309,8 +639,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); @@ -370,13 +700,16 @@ public function update(int $contextId, string $userId, ?string $name, ?string $i $context->setNodes($currentNodes); $context->setPages($currentPages); } - $this->auditLogService->log(Operation::ContextUpdated, 'Tables application with ID %s was updated', [ - 'contextId' => $contextId, + if ($menuItems !== null) { + $this->replaceMenu($context, $menuItems, $userId); + } + $changes = [ 'addedNodes' => $addedNodes, 'removedNodes' => $removedNodes, 'updatedNodes' => $updatedNodes, - ]); + ]; + $this->shapeAddedForResponse($context, $knownNodeIds, $knownContentIds, false); return $context; } @@ -621,9 +954,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(); } } @@ -647,7 +978,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 53dafa9f2e..7a946043c4 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 = []; @@ -73,7 +77,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(AuditLogServiceInterface::class), $this->connectionAdapter, @@ -81,11 +85,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, ); } @@ -198,4 +203,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 1aef07dd6a..962ba1bbdf 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -9,12 +9,15 @@ namespace OCA\Tables\Service; +use InvalidArgumentException; +use OC\DB\Exceptions\DbalException; use OCA\Tables\AppInfo\Application; use OCA\Tables\AuditLog\Operation; 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; @@ -22,8 +25,12 @@ 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\Service\Support\AuditLogServiceInterface; +use OCA\Tables\Tests\Unit\Database\DbErrorStub; use OCP\IDBConnection; use OCP\IL10N; use OCP\INavigationManager; @@ -46,6 +53,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(); @@ -56,6 +65,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); $this->userManager = $this->createMock(IUserManager::class); $this->auditLogService = $this->createMock(AuditLogServiceInterface::class); $this->dbConnection = $this->createMock(IDBConnection::class); @@ -63,6 +74,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); @@ -81,11 +94,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, ); } @@ -506,6 +520,483 @@ public function testImportedPages(array $pages, array $expected): void { self::assertSame($expected, $inserted); } + 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; @@ -677,4 +1168,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 eb6d3f4c8eb38cc97189ba1de9e8a0c5e81d6d07 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 22 Oct 2026 15:10:24 +0200 Subject: [PATCH 4/4] 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 8337c1cc33..3165a3b0a1 100644 --- a/openapi.json +++ b/openapi.json @@ -12592,6 +12592,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" + } + } + } } } } @@ -13024,6 +13066,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 ab6ba6e74f..e092878ad8 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -7228,6 +7228,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; }; }; }; @@ -7418,6 +7433,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; }; }; };