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/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; 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); + } }