From 911d3e4184b4ded69f3897825bb93b10a0d75e1c Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 22 Oct 2026 15:10:23 +0200 Subject: [PATCH 1/2] 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 +++++++++ .../Version2400Date20261022010000.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/Version2400Date20261022010000.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 c8624d51d7..ffe942925d 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); } @@ -77,7 +78,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'], @@ -146,6 +150,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); } @@ -159,26 +170,9 @@ public function findAll(?string $userId = null): array { $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 ((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); @@ -209,24 +203,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; } @@ -245,7 +242,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])); } /** @@ -261,25 +258,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/Version2400Date20261022010000.php b/lib/Migration/Version2400Date20261022010000.php new file mode 100644 index 0000000000..605cb81ebb --- /dev/null +++ b/lib/Migration/Version2400Date20261022010000.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 f58ba28fb9..64d3a92fc2 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -228,6 +228,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, @@ -235,6 +250,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 b71ac77127..d8dce10913 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.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; @@ -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); + } } /** @@ -319,8 +365,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); @@ -328,7 +378,6 @@ public function delete(int $contextId, string $userId): Context { $this->pageContentMapper->deleteByPageId($pageId); $this->pageMapper->deleteByPageId($pageId); } - $this->contextMapper->delete($context); }, $this->dbc); return $context; } diff --git a/lib/Service/ShareService.php b/lib/Service/ShareService.php index cbbb649e05..52a046a87c 100644 --- a/lib/Service/ShareService.php +++ b/lib/Service/ShareService.php @@ -835,18 +835,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 6d1cc09870..b50f1df299 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(); @@ -84,6 +89,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 1f4b7afb2b..d2800fafad 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( @@ -83,6 +84,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 5fb9d9df5c..057c94205b 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -13,6 +13,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; @@ -37,6 +38,9 @@ final class ContextServiceTest extends TestCase { private MockObject $pageMapper; private MockObject $pageContentMapper; private MockObject $widgetService; + private MockObject $menuItemMapper; + private MockObject $permissionsService; + private MockObject $shareService; private MockObject $dbConnection; protected function setUp(): void { @@ -47,14 +51,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); $userManager = $this->createMock(IUserManager::class); $eventDispatcher = $this->createMock(IEventDispatcher::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); @@ -64,11 +69,11 @@ protected function setUp(): void { $this->pageMapper, $this->pageContentMapper, $logger, - $permissionsService, + $this->permissionsService, $userManager, $eventDispatcher, $this->dbConnection, - $shareService, + $this->shareService, false, $navigationManager, $urlGenerator, @@ -76,6 +81,7 @@ protected function setUp(): void { $this->createMock(ViewMapper::class), $this->widgetService, new GridWidgetTypes($l10n), + $this->menuItemMapper, ); } @@ -179,6 +185,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]); @@ -567,4 +602,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 ce37baf15f..b107e1d54d 100644 --- a/tests/unit/Service/ShareServiceTest.php +++ b/tests/unit/Service/ShareServiceTest.php @@ -247,4 +247,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 c7a6e95f96b319c07626955d841a7299a9ce4fc2 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 22 Oct 2026 15:10:23 +0200 Subject: [PATCH 2/2] 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 53f5ab0887..6cda2d1a84 100644 --- a/openapi.json +++ b/openapi.json @@ -301,6 +301,12 @@ "ownerType": { "type": "integer", "format": "int64" + }, + "menuItems": { + "type": "array", + "items": { + "$ref": "#/components/schemas/MenuItem" + } } } }, @@ -408,6 +414,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 aa641b49f9..7c51a21974 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1200,6 +1200,7 @@ export type components = { readonly owner: string; /** Format: int64 */ readonly ownerType: number; + readonly menuItems?: readonly components["schemas"]["MenuItem"][]; }; readonly ContextNavigation: { /** Format: int64 */ @@ -1234,6 +1235,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;