From bc984d8ef5ff31f4f4a0d9f5358c503d70093f28 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 16 Oct 2026 09:23:59 +0200 Subject: [PATCH 1/3] feat(pages): store grid pages and their widgets relationally Adds the data model of nextcloud/tables#3100: pages get a title, emoji and description and the grid page type; widgets, their settings and the node a data widget shows live in tables_contexts_widget, tables_contexts_widget_setting and tables_contexts_widget_source. Deleting an application or page deletes its widgets; removing a node deletes only the widget sources. Scheme and user migration export carry the widgets, import remaps their node. Assisted-by: ClaudeCode:claude-opus-5-5 --- appinfo/info.xml | 2 +- lib/Db/ContextMapper.php | 7 +- lib/Db/Page.php | 13 ++ lib/Db/Widget.php | 62 +++++++ lib/Db/WidgetMapper.php | 78 ++++++++ lib/Db/WidgetSetting.php | 33 ++++ lib/Db/WidgetSettingMapper.php | 88 +++++++++ lib/Db/WidgetSource.php | 31 ++++ lib/Db/WidgetSourceMapper.php | 99 +++++++++++ .../Version2400Date20261016000001.php | 98 ++++++++++ lib/Service/ContextService.php | 57 +++++- lib/Service/WidgetService.php | 124 +++++++++++++ lib/UserMigration/TablesMigrator.php | 3 + tests/unit/Db/ContextMapperTest.php | 122 +++++++++++++ .../Service/ContextPagesRoundTripTest.php | 168 ++++++++++++++++++ tests/unit/Service/ContextServiceTest.php | 108 ++++++++++- tests/unit/Service/WidgetServiceTest.php | 139 +++++++++++++++ tests/unit/TablesMigratorTest.php | 29 +++ 18 files changed, 1255 insertions(+), 6 deletions(-) create mode 100644 lib/Db/Widget.php create mode 100644 lib/Db/WidgetMapper.php create mode 100644 lib/Db/WidgetSetting.php create mode 100644 lib/Db/WidgetSettingMapper.php create mode 100644 lib/Db/WidgetSource.php create mode 100644 lib/Db/WidgetSourceMapper.php create mode 100644 lib/Migration/Version2400Date20261016000001.php create mode 100644 lib/Service/WidgetService.php create mode 100644 tests/unit/Db/ContextMapperTest.php create mode 100644 tests/unit/Service/ContextPagesRoundTripTest.php create mode 100644 tests/unit/Service/WidgetServiceTest.php diff --git a/appinfo/info.xml b/appinfo/info.xml index 8545043c14..61b25aa33b 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -26,7 +26,7 @@ Share your tables and views with users and groups within your cloud. Have a good time and manage whatever you want. ]]> - 2.4.0-dev.0 + 2.4.0-dev.5 AGPL-3.0-or-later Nextcloud GmbH and Nextcloud contributors Tables diff --git a/lib/Db/ContextMapper.php b/lib/Db/ContextMapper.php index 0da199335e..71e13a5ff4 100644 --- a/lib/Db/ContextMapper.php +++ b/lib/Db/ContextMapper.php @@ -37,7 +37,7 @@ protected function getFindContextBaseQuery(?string $userId): IQueryBuilder { $qb->select( 'c.*', 'r.id as node_rel_id', 'r.node_id', 'r.node_type', 'r.permissions', - 'p.id as page_id', 'p.page_type', + 'p.id as page_id', 'p.page_type', 'p.title as page_title', 'p.emoji as page_emoji', 'p.description as page_description', 'pc.id as content_id', 'pc.order', 'n.display_mode as display_mode_default', 's.id as share_id', 's.receiver', 's.receiver_type' @@ -131,7 +131,10 @@ protected function formatResultRows(array $rows, ?string $userId) { $carry[$item['page_id']] ??= ['content' => []]; $carry[$item['page_id']]['id'] = (int)$item['page_id']; $carry[$item['page_id']]['page_type'] = $item['page_type']; - if ($item['node_rel_id'] !== null) { + $carry[$item['page_id']]['title'] = $item['page_title']; + $carry[$item['page_id']]['emoji'] = $item['page_emoji']; + $carry[$item['page_id']]['description'] = $item['page_description']; + if ($item['node_rel_id'] !== null && $item['content_id'] !== null) { $carry[$item['page_id']]['content'][$item['content_id']] = [ 'order' => (int)$item['order'], 'node_rel_id' => (int)$item['node_rel_id'] diff --git a/lib/Db/Page.php b/lib/Db/Page.php index 5ceeb1bd00..5e18089da3 100644 --- a/lib/Db/Page.php +++ b/lib/Db/Page.php @@ -15,12 +15,22 @@ * @method setContextId(int $value): void * @method getPageType(): string * @method setPageType(string $value): void + * @method getTitle(): ?string + * @method setTitle(?string $value): void + * @method getEmoji(): ?string + * @method setEmoji(?string $value): void + * @method getDescription(): ?string + * @method setDescription(?string $value): void */ class Page extends Entity implements \JsonSerializable { public const TYPE_STARTPAGE = 'startpage'; + public const TYPE_GRID = 'grid'; protected ?int $contextId = null; protected ?string $pageType = null; + protected ?string $title = null; + protected ?string $emoji = null; + protected ?string $description = null; public function __construct() { $this->addType('id', 'integer'); @@ -31,6 +41,9 @@ public function jsonSerialize(): array { 'id' => $this->getId(), 'contextId' => $this->getContextId(), 'pageType' => $this->getPageType(), + 'title' => $this->getTitle(), + 'emoji' => $this->getEmoji(), + 'description' => $this->getDescription(), ]; } } diff --git a/lib/Db/Widget.php b/lib/Db/Widget.php new file mode 100644 index 0000000000..ee657cec71 --- /dev/null +++ b/lib/Db/Widget.php @@ -0,0 +1,62 @@ +addType('id', 'integer'); + $this->addType('page_id', 'integer'); + $this->addType('grid_x', 'integer'); + $this->addType('grid_y', 'integer'); + $this->addType('grid_width', 'integer'); + $this->addType('grid_height', 'integer'); + } + + public function jsonSerialize(): array { + return [ + 'id' => $this->getId(), + 'pageId' => $this->getPageId(), + 'type' => $this->getType(), + 'title' => $this->getTitle(), + 'gridX' => $this->getGridX(), + 'gridY' => $this->getGridY(), + 'gridWidth' => $this->getGridWidth(), + 'gridHeight' => $this->getGridHeight(), + ]; + } +} diff --git a/lib/Db/WidgetMapper.php b/lib/Db/WidgetMapper.php new file mode 100644 index 0000000000..9843b30c77 --- /dev/null +++ b/lib/Db/WidgetMapper.php @@ -0,0 +1,78 @@ + */ +class WidgetMapper extends QBMapper { + protected string $table = 'tables_contexts_widget'; + + public function __construct(IDBConnection $db) { + parent::__construct($db, $this->table, Widget::class); + } + + /** + * @param int[] $pageIds + * @return Widget[] ordered by page and grid position + * @throws Exception + */ + public function findByPageIds(array $pageIds): array { + if ($pageIds === []) { + return []; + } + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->table) + ->where($qb->expr()->in('page_id', $qb->createNamedParameter($pageIds, IQueryBuilder::PARAM_INT_ARRAY))) + ->orderBy('page_id', 'ASC') + ->addOrderBy('grid_y', 'ASC') + ->addOrderBy('grid_x', 'ASC') + ->addOrderBy('id', 'ASC'); + + return $this->findEntities($qb); + } + + /** + * @return int[] + * @throws Exception + */ + public function findIdsByPageId(int $pageId): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('id') + ->from($this->table) + ->where($qb->expr()->eq('page_id', $qb->createNamedParameter($pageId, IQueryBuilder::PARAM_INT))); + + $result = $qb->executeQuery(); + $ids = []; + while ($row = $result->fetchAssociative()) { + $ids[] = (int)$row['id']; + } + $result->closeCursor(); + return $ids; + } + + /** + * @param int[] $widgetIds + * @throws Exception + */ + public function deleteByIds(array $widgetIds): void { + if ($widgetIds === []) { + return; + } + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->table) + ->where($qb->expr()->in('id', $qb->createNamedParameter($widgetIds, IQueryBuilder::PARAM_INT_ARRAY))) + ->executeStatement(); + } +} diff --git a/lib/Db/WidgetSetting.php b/lib/Db/WidgetSetting.php new file mode 100644 index 0000000000..983188dec9 --- /dev/null +++ b/lib/Db/WidgetSetting.php @@ -0,0 +1,33 @@ +addType('id', 'integer'); + $this->addType('widget_id', 'integer'); + } +} diff --git a/lib/Db/WidgetSettingMapper.php b/lib/Db/WidgetSettingMapper.php new file mode 100644 index 0000000000..7b2c834e1a --- /dev/null +++ b/lib/Db/WidgetSettingMapper.php @@ -0,0 +1,88 @@ + value maps. Values are stored JSON + * encoded, one row per setting. + * + * @psalm-type TablesWidgetSettingValue = string|int|float|bool|null + * @template-extends QBMapper + */ +class WidgetSettingMapper extends QBMapper { + protected string $table = 'tables_contexts_widget_setting'; + + public function __construct(IDBConnection $db) { + parent::__construct($db, $this->table, WidgetSetting::class); + } + + /** + * @param int[] $widgetIds + * @return array> settings by widget id + * @throws Exception + */ + public function findSettingsByWidgetIds(array $widgetIds): array { + if ($widgetIds === []) { + return []; + } + $qb = $this->db->getQueryBuilder(); + $qb->select('widget_id', 'name', 'value') + ->from($this->table) + ->where($qb->expr()->in('widget_id', $qb->createNamedParameter($widgetIds, IQueryBuilder::PARAM_INT_ARRAY))) + ->orderBy('id', 'ASC'); + + $result = $qb->executeQuery(); + $settings = []; + while ($row = $result->fetchAssociative()) { + $settings[(int)$row['widget_id']][(string)$row['name']] = $row['value'] === null + ? null + : json_decode((string)$row['value'], true); + } + $result->closeCursor(); + return $settings; + } + + /** + * Replaces all settings of a widget. + * + * @param array $settings + * @throws Exception + * @throws \JsonException + */ + public function replaceSettings(int $widgetId, array $settings): void { + $this->deleteByWidgetIds([$widgetId]); + foreach ($settings as $name => $value) { + $setting = new WidgetSetting(); + $setting->setWidgetId($widgetId); + $setting->setName((string)$name); + $setting->setValue($value === null ? null : json_encode($value, JSON_THROW_ON_ERROR)); + $this->insert($setting); + } + } + + /** + * @param int[] $widgetIds + * @throws Exception + */ + public function deleteByWidgetIds(array $widgetIds): void { + if ($widgetIds === []) { + return; + } + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->table) + ->where($qb->expr()->in('widget_id', $qb->createNamedParameter($widgetIds, IQueryBuilder::PARAM_INT_ARRAY))) + ->executeStatement(); + } +} diff --git a/lib/Db/WidgetSource.php b/lib/Db/WidgetSource.php new file mode 100644 index 0000000000..64fc9a49f2 --- /dev/null +++ b/lib/Db/WidgetSource.php @@ -0,0 +1,31 @@ +addType('widget_id', 'integer'); + $this->addType('node_rel_id', 'integer'); + } +} diff --git a/lib/Db/WidgetSourceMapper.php b/lib/Db/WidgetSourceMapper.php new file mode 100644 index 0000000000..9041e7d849 --- /dev/null +++ b/lib/Db/WidgetSourceMapper.php @@ -0,0 +1,99 @@ + + */ +class WidgetSourceMapper extends QBMapper { + protected string $table = 'tables_contexts_widget_source'; + + public function __construct(IDBConnection $db) { + parent::__construct($db, $this->table, WidgetSource::class); + } + + /** + * @param int[] $widgetIds + * @return array node relation ids by widget id + * @throws Exception + */ + public function findNodeRelIdsByWidgetIds(array $widgetIds): array { + if ($widgetIds === []) { + return []; + } + $qb = $this->db->getQueryBuilder(); + $qb->select('widget_id', 'node_rel_id') + ->from($this->table) + ->where($qb->expr()->in('widget_id', $qb->createNamedParameter($widgetIds, IQueryBuilder::PARAM_INT_ARRAY))); + + $result = $qb->executeQuery(); + $sources = []; + while ($row = $result->fetchAssociative()) { + $sources[(int)$row['widget_id']] = (int)$row['node_rel_id']; + } + $result->closeCursor(); + return $sources; + } + + /** + * Points a widget at a node relation, or removes its source when null. + * + * @throws Exception + */ + public function setSource(int $widgetId, ?int $nodeRelId): void { + $this->deleteByWidgetIds([$widgetId]); + if ($nodeRelId === null) { + return; + } + $qb = $this->db->getQueryBuilder(); + $qb->insert($this->table) + ->values([ + 'widget_id' => $qb->createNamedParameter($widgetId, IQueryBuilder::PARAM_INT), + 'node_rel_id' => $qb->createNamedParameter($nodeRelId, IQueryBuilder::PARAM_INT), + ]) + ->executeStatement(); + } + + /** + * @param int[] $widgetIds + * @throws Exception + */ + public function deleteByWidgetIds(array $widgetIds): void { + if ($widgetIds === []) { + return; + } + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->table) + ->where($qb->expr()->in('widget_id', $qb->createNamedParameter($widgetIds, IQueryBuilder::PARAM_INT_ARRAY))) + ->executeStatement(); + } + + /** + * @param int[] $nodeRelIds + * @throws Exception + */ + public function deleteByNodeRelIds(array $nodeRelIds): void { + if ($nodeRelIds === []) { + return; + } + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->table) + ->where($qb->expr()->in('node_rel_id', $qb->createNamedParameter($nodeRelIds, IQueryBuilder::PARAM_INT_ARRAY))) + ->executeStatement(); + } +} diff --git a/lib/Migration/Version2400Date20261016000001.php b/lib/Migration/Version2400Date20261016000001.php new file mode 100644 index 0000000000..f63f37918b --- /dev/null +++ b/lib/Migration/Version2400Date20261016000001.php @@ -0,0 +1,98 @@ +extendPageTable($schema); + $this->haveWidgetTable($schema); + $this->haveWidgetSettingTable($schema); + $this->haveWidgetSourceTable($schema); + + return $schema; + } + + private function extendPageTable(ISchemaWrapper $schema): void { + if (!$schema->hasTable(self::PREFIX . 'page')) { + return; + } + $table = $schema->getTable(self::PREFIX . 'page'); + if (!$table->hasColumn('title')) { + $table->addColumn('title', Types::STRING, ['notnull' => false, 'length' => 200]); + } + if (!$table->hasColumn('emoji')) { + $table->addColumn('emoji', Types::STRING, ['notnull' => false, 'length' => 64]); + } + if (!$table->hasColumn('description')) { + $table->addColumn('description', Types::TEXT, ['notnull' => false]); + } + if (!$table->hasIndex('tables_ctx_page_ctx_idx')) { + $table->addIndex(['context_id'], 'tables_ctx_page_ctx_idx'); + } + } + + private function haveWidgetTable(ISchemaWrapper $schema): void { + if ($schema->hasTable(self::PREFIX . 'widget')) { + return; + } + $table = $schema->createTable(self::PREFIX . 'widget'); + $table->addColumn('id', Types::INTEGER, ['autoincrement' => true, 'notnull' => true]); + $table->addColumn('page_id', Types::INTEGER, ['notnull' => true]); + $table->addColumn('type', Types::STRING, ['notnull' => true, 'length' => 64]); + $table->addColumn('title', Types::STRING, ['notnull' => false, 'length' => 200]); + $table->addColumn('grid_x', Types::SMALLINT, ['notnull' => true, 'default' => 0]); + $table->addColumn('grid_y', Types::INTEGER, ['notnull' => true, 'default' => 0]); + $table->addColumn('grid_width', Types::SMALLINT, ['notnull' => true, 'default' => 1]); + $table->addColumn('grid_height', Types::SMALLINT, ['notnull' => true, 'default' => 1]); + $table->setPrimaryKey(['id']); + $table->addIndex(['page_id'], 'tables_ctx_widget_page_idx'); + } + + private function haveWidgetSettingTable(ISchemaWrapper $schema): void { + if ($schema->hasTable(self::PREFIX . 'widget_setting')) { + return; + } + $table = $schema->createTable(self::PREFIX . 'widget_setting'); + $table->addColumn('id', Types::INTEGER, ['autoincrement' => true, 'notnull' => true]); + $table->addColumn('widget_id', Types::INTEGER, ['notnull' => true]); + $table->addColumn('name', Types::STRING, ['notnull' => true, 'length' => 64]); + $table->addColumn('value', Types::TEXT, ['notnull' => false]); + $table->setPrimaryKey(['id']); + $table->addUniqueIndex(['widget_id', 'name'], 'tables_ctx_wsetting_uq'); + } + + private function haveWidgetSourceTable(ISchemaWrapper $schema): void { + if ($schema->hasTable(self::PREFIX . 'widget_source')) { + return; + } + $table = $schema->createTable(self::PREFIX . 'widget_source'); + $table->addColumn('widget_id', Types::INTEGER, ['notnull' => true]); + $table->addColumn('node_rel_id', Types::INTEGER, ['notnull' => true]); + $table->setPrimaryKey(['widget_id']); + $table->addIndex(['node_rel_id'], 'tables_ctx_wsource_node_idx'); + } +} diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 468d3de83e..c05283ce6f 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -62,6 +62,7 @@ public function __construct( protected IURLGenerator $urlGenerator, private TableMapper $tableMapper, private ViewMapper $viewMapper, + private WidgetService $widgetService, ) { } @@ -319,6 +320,7 @@ public function delete(int $contextId, string $userId): Context { $this->shareService->deleteAllForContext($context); $this->contextNodeRelMapper->deleteAllByContextId($context->getId()); $pageIds = $this->pageMapper->getPageIdsForContext($context->getId()); + $this->widgetService->deleteByPageIds($pageIds); foreach ($pageIds as $pageId) { $this->pageContentMapper->deleteByPageId($pageId); $this->pageMapper->deleteByPageId($pageId); @@ -332,6 +334,7 @@ public function deleteNodeRel(int $nodeId, int $nodeType): void { try { $nodeRelIds = $this->contextNodeRelMapper->getRelIdsForNode($nodeId, $nodeType); $this->atomic(function () use ($nodeRelIds): void { + $this->widgetService->deleteSourcesByNodeRelIds($nodeRelIds); $this->pageContentMapper->deleteByNodeRelIds($nodeRelIds); $this->contextNodeRelMapper->deleteByNodeRelIds($nodeRelIds); }, $this->dbc); @@ -432,6 +435,7 @@ public function addNodeToContextAndStartpage(int $contextId, int $nodeId, int $n */ public function removeNodeFromContextAndPages(int $nodeRelationId): array { $nodeRelation = $this->removeNodeFromContextById($nodeRelationId); + $this->widgetService->deleteSourcesByNodeRelIds([$nodeRelationId]); $contents = $this->removeNodeRelFromAllPages($nodeRelation); return [$nodeRelation, $contents]; } @@ -635,6 +639,9 @@ public function importContext(Table $table, array $context, int $tableOldId): ?C $page = new Page(); $page->setContextId($newContext->getId()); $page->setPageType($pageArr['page_type'] ?? $pageArr['pageType'] ?? ''); + $page->setTitle($pageArr['title'] ?? null); + $page->setEmoji($pageArr['emoji'] ?? null); + $page->setDescription($pageArr['description'] ?? null); $page = $this->pageMapper->insert($page); $pageMap[$pageArr['id'] ?? null] = $page; @@ -650,12 +657,60 @@ public function importContext(Table $table, array $context, int $tableOldId): ?C $this->pageContentMapper->insert($pageContent); } } + + foreach ($pageArr['widgets'] ?? [] as $widgetArr) { + $this->importWidget($page->getId(), $widgetArr, $nodeRelMap); + } } } return $newContext; } + /** + * The pages of an application with the widgets of each page under `widgets`. + * + * @throws Exception + */ + public function getPagesWithWidgets(Context $context): array { + $pages = $context->getPages() ?? []; + $widgets = $this->widgetService->findByPageIds(array_keys($pages)); + foreach ($pages as $pageId => $page) { + $pages[$pageId]['widgets'] = $widgets[$pageId] ?? []; + } + return $pages; + } + + /** + * Inserts an exported widget on a page. Its source is remapped to the imported node + * relation, and dropped when that node was not imported. + * + * @param array $nodeRelMap imported node relations by their exported id + * @throws Exception + * @throws \JsonException + */ + private function importWidget(int $pageId, mixed $widgetArr, array $nodeRelMap): void { + if (!is_array($widgetArr) || !is_string($widgetArr['type'] ?? null)) { + return; + } + $oldNodeRelId = $widgetArr['source']['nodeRelId'] ?? null; + $newNodeRel = $oldNodeRelId !== null ? ($nodeRelMap[$oldNodeRelId] ?? null) : null; + $settings = is_array($widgetArr['settings'] ?? null) ? $widgetArr['settings'] : []; + $this->widgetService->insert( + $pageId, + $widgetArr['type'], + is_string($widgetArr['title'] ?? null) ? $widgetArr['title'] : null, + [ + 'gridX' => (int)($widgetArr['gridX'] ?? 0), + 'gridY' => (int)($widgetArr['gridY'] ?? 0), + 'gridWidth' => max(1, (int)($widgetArr['gridWidth'] ?? 1)), + 'gridHeight' => max(1, (int)($widgetArr['gridHeight'] ?? 1)), + ], + $settings, + $newNodeRel?->getId(), + ); + } + /** * @param int $contextId * @@ -705,7 +760,7 @@ public function getScheme(int $contextId, string $userId): ContextScheme { $context->getIcon(), $context->getDescription(), array_values($nodes), - $context->getPages(), + $this->getPagesWithWidgets($context), array_values($tables), ); } diff --git a/lib/Service/WidgetService.php b/lib/Service/WidgetService.php new file mode 100644 index 0000000000..8279d6e26b --- /dev/null +++ b/lib/Service/WidgetService.php @@ -0,0 +1,124 @@ +, source: ?array{nodeRelId: int}} + */ +class WidgetService { + public function __construct( + private readonly WidgetMapper $widgetMapper, + private readonly WidgetSettingMapper $widgetSettingMapper, + private readonly WidgetSourceMapper $widgetSourceMapper, + ) { + } + + /** + * @param int[] $pageIds + * @return array> widgets by page id, in grid order + * @throws Exception + */ + public function findByPageIds(array $pageIds): array { + $widgets = $this->widgetMapper->findByPageIds($pageIds); + $widgetIds = array_map(static fn (Widget $widget): int => $widget->getId(), $widgets); + $settings = $this->widgetSettingMapper->findSettingsByWidgetIds($widgetIds); + $sources = $this->widgetSourceMapper->findNodeRelIdsByWidgetIds($widgetIds); + + $byPage = []; + foreach ($widgets as $widget) { + $id = $widget->getId(); + $byPage[$widget->getPageId()][] = [ + 'id' => $id, + 'type' => $widget->getType(), + 'title' => $widget->getTitle(), + 'gridX' => $widget->getGridX(), + 'gridY' => $widget->getGridY(), + 'gridWidth' => $widget->getGridWidth(), + 'gridHeight' => $widget->getGridHeight(), + 'settings' => $settings[$id] ?? [], + 'source' => isset($sources[$id]) ? ['nodeRelId' => $sources[$id]] : null, + ]; + } + return $byPage; + } + + /** + * @param array{gridX: int, gridY: int, gridWidth: int, gridHeight: int} $position + * @param array $settings + * @throws Exception + * @throws \JsonException + */ + public function insert(int $pageId, string $type, ?string $title, array $position, array $settings, ?int $nodeRelId): Widget { + $widget = new Widget(); + $widget->setPageId($pageId); + $this->applyFields($widget, $type, $title, $position); + $widget = $this->widgetMapper->insert($widget); + + $this->widgetSettingMapper->replaceSettings($widget->getId(), $settings); + $this->widgetSourceMapper->setSource($widget->getId(), $nodeRelId); + return $widget; + } + + /** + * Deletes the widgets of the given pages with their settings and sources. + * + * @param int[] $pageIds + * @throws Exception + */ + public function deleteByPageIds(array $pageIds): void { + $widgetIds = []; + foreach ($pageIds as $pageId) { + $widgetIds = array_merge($widgetIds, $this->widgetMapper->findIdsByPageId($pageId)); + } + $this->deleteByIds($widgetIds); + } + + /** + * @param int[] $widgetIds + * @throws Exception + */ + public function deleteByIds(array $widgetIds): void { + $this->widgetSettingMapper->deleteByWidgetIds($widgetIds); + $this->widgetSourceMapper->deleteByWidgetIds($widgetIds); + $this->widgetMapper->deleteByIds($widgetIds); + } + + /** + * Unlinks every widget that shows one of the given node relations. The widgets stay. + * + * @param int[] $nodeRelIds + * @throws Exception + */ + public function deleteSourcesByNodeRelIds(array $nodeRelIds): void { + $this->widgetSourceMapper->deleteByNodeRelIds($nodeRelIds); + } + + /** + * @param array{gridX: int, gridY: int, gridWidth: int, gridHeight: int} $position + */ + private function applyFields(Widget $widget, string $type, ?string $title, array $position): void { + $widget->setType($type); + $widget->setTitle($title); + $widget->setGridX($position['gridX']); + $widget->setGridY($position['gridY']); + $widget->setGridWidth($position['gridWidth']); + $widget->setGridHeight($position['gridHeight']); + } +} diff --git a/lib/UserMigration/TablesMigrator.php b/lib/UserMigration/TablesMigrator.php index ec4d528307..29edfa77b2 100644 --- a/lib/UserMigration/TablesMigrator.php +++ b/lib/UserMigration/TablesMigrator.php @@ -122,6 +122,9 @@ public function export( $exportDestination->addFileContents(self::FILE_VIEWS, json_encode($views)); $contexts = $this->contextMapper->findAll($uid); + foreach ($contexts as $context) { + $context->setPages($this->contextService->getPagesWithWidgets($context)); + } $exportDestination->addFileContents(self::FILE_CONTEXTS, json_encode($contexts)); $contextIds = array_map(fn ($c) => $c->getId(), $contexts); diff --git a/tests/unit/Db/ContextMapperTest.php b/tests/unit/Db/ContextMapperTest.php new file mode 100644 index 0000000000..013a3ead18 --- /dev/null +++ b/tests/unit/Db/ContextMapperTest.php @@ -0,0 +1,122 @@ +createMock(UserHelper::class); + $userHelper->method('getGroupIdsForUser')->willReturn([]); + $this->mapper = new ContextMapper($this->connectionAdapter, $userHelper, $this->createMock(GroupHelper::class)); + $this->nodeRelMapper = new ContextNodeRelationMapper($this->connectionAdapter); + $this->pageMapper = new PageMapper($this->connectionAdapter); + $this->pageContentMapper = new PageContentMapper($this->connectionAdapter); + } + + protected function tearDown(): void { + foreach ($this->contexts as $context) { + foreach ($this->pageMapper->getPageIdsForContext($context->getId()) as $pageId) { + $this->pageContentMapper->deleteByPageId($pageId); + $this->pageMapper->deleteByPageId($pageId); + } + $this->nodeRelMapper->deleteAllByContextId($context->getId()); + $this->mapper->delete($context); + } + parent::tearDown(); + } + + public function testGridPageHasItsFieldsAndNoStartPageContent(): void { + $context = $this->createContext(); + $first = $this->addNode($context, 11); + $second = $this->addNode($context, 12); + $startPage = $this->addPage($context, Page::TYPE_STARTPAGE, null, null, null); + $firstContent = $this->addContent($startPage, $first, 10); + $secondContent = $this->addContent($startPage, $second, 20); + $gridPage = $this->addPage($context, Page::TYPE_GRID, 'Overview', '📊', 'Open work at a glance'); + + $pages = $this->mapper->findById($context->getId(), self::OWNER)->getPages(); + + $this->assertEqualsCanonicalizing([$startPage->getId(), $gridPage->getId()], array_keys($pages)); + $this->assertSame([ + 'content' => [], + 'id' => $gridPage->getId(), + 'page_type' => Page::TYPE_GRID, + 'title' => 'Overview', + 'emoji' => '📊', + 'description' => 'Open work at a glance', + ], $pages[$gridPage->getId()]); + $this->assertNull($pages[$startPage->getId()]['title']); + $this->assertSame([$firstContent->getId(), $secondContent->getId()], array_keys($pages[$startPage->getId()]['content'])); + $this->assertSame([$first->getId(), $second->getId()], array_keys($this->mapper->findById($context->getId(), self::OWNER)->getNodes())); + } + + private function createContext(): Context { + $context = new Context(); + $context->setName('Mapper test'); + $context->setIcon('tables'); + $context->setDescription(''); + $context->setOwnerId(self::OWNER); + $context->setOwnerType(Application::OWNER_TYPE_USER); + $this->contexts[] = $context = $this->mapper->insert($context); + return $context; + } + + private function addNode(Context $context, int $nodeId): ContextNodeRelation { + $relation = new ContextNodeRelation(); + $relation->setContextId($context->getId()); + $relation->setNodeId($nodeId); + $relation->setNodeType(Application::NODE_TYPE_TABLE); + $relation->setPermissions(Application::PERMISSION_READ); + return $this->nodeRelMapper->insert($relation); + } + + private function addPage(Context $context, string $type, ?string $title, ?string $emoji, ?string $description): Page { + $page = new Page(); + $page->setContextId($context->getId()); + $page->setPageType($type); + $page->setTitle($title); + $page->setEmoji($emoji); + $page->setDescription($description); + return $this->pageMapper->insert($page); + } + + private function addContent(Page $page, ContextNodeRelation $relation, int $order): PageContent { + $content = new PageContent(); + $content->setPageId($page->getId()); + $content->setNodeRelId($relation->getId()); + $content->setOrder($order); + return $this->pageContentMapper->insert($content); + } +} diff --git a/tests/unit/Service/ContextPagesRoundTripTest.php b/tests/unit/Service/ContextPagesRoundTripTest.php new file mode 100644 index 0000000000..a7a197efbc --- /dev/null +++ b/tests/unit/Service/ContextPagesRoundTripTest.php @@ -0,0 +1,168 @@ +createMock(UserHelper::class); + $userHelper->method('getGroupIdsForUser')->willReturn([]); + $this->contextMapper = new ContextMapper($this->connectionAdapter, $userHelper, $this->createMock(GroupHelper::class)); + $this->nodeRelMapper = new ContextNodeRelationMapper($this->connectionAdapter); + $this->pageMapper = new PageMapper($this->connectionAdapter); + $this->widgetService = new WidgetService( + new WidgetMapper($this->connectionAdapter), + new WidgetSettingMapper($this->connectionAdapter), + new WidgetSourceMapper($this->connectionAdapter), + ); + $this->service = new ContextService( + $this->contextMapper, + $this->nodeRelMapper, + $this->pageMapper, + new PageContentMapper($this->connectionAdapter), + $this->createMock(LoggerInterface::class), + $this->createMock(PermissionsService::class), + $this->createMock(IUserManager::class), + $this->createMock(IEventDispatcher::class), + $this->connectionAdapter, + $this->createMock(ShareService::class), + false, + $this->createMock(INavigationManager::class), + $this->createMock(IURLGenerator::class), + $this->createMock(TableMapper::class), + $this->createMock(ViewMapper::class), + $this->widgetService, + ); + } + + protected function tearDown(): void { + foreach ($this->contextIds as $contextId) { + try { + $this->service->delete($contextId, self::OWNER); + } catch (\Throwable) { + } + } + parent::tearDown(); + } + + public function testExportedWidgetsAreImportedWithTheirNode(): void { + [$context, $relation, $gridPageId] = $this->createContextWithGridPage(); + $table = new Table(); + $table->id = 4242; + $table->setOwnership(self::OWNER); + + $exported = json_decode(json_encode(array_merge( + $this->contextMapper->findById($context->getId(), self::OWNER)->jsonSerialize(), + ['pages' => $this->service->getPagesWithWidgets($this->contextMapper->findById($context->getId(), self::OWNER))], + )), true); + $exported['nodes'][$relation->getId()]['node_id'] = 4242; + + $imported = $this->service->importContext($table, $exported, 4242); + $this->assertNotNull($imported); + $this->contextIds[] = $imported->getId(); + + $importedContext = $this->contextMapper->findById($imported->getId(), self::OWNER); + $importedRelationId = array_key_first($importedContext->getNodes()); + $importedPages = $this->service->getPagesWithWidgets($importedContext); + $gridPages = array_values(array_filter($importedPages, static fn (array $page): bool => $page['page_type'] === Page::TYPE_GRID)); + $this->assertCount(1, $gridPages); + $this->assertSame(['Overview', '📊', 'At a glance'], [$gridPages[0]['title'], $gridPages[0]['emoji'], $gridPages[0]['description']]); + + $original = $this->service->getPagesWithWidgets($this->contextMapper->findById($context->getId(), self::OWNER))[$gridPageId]['widgets']; + $withoutIds = static fn (array $widgets): array => array_map(static function (array $widget): array { + unset($widget['id']); + return $widget; + }, $widgets); + $expected = $withoutIds($original); + $expected[1]['source'] = ['nodeRelId' => $importedRelationId]; + $this->assertNotSame($relation->getId(), $importedRelationId); + $this->assertSame($expected, $withoutIds($gridPages[0]['widgets'])); + } + + public function testRemovingTheNodeKeepsTheWidgetAndDeletingTheContextRemovesIt(): void { + [$context, $relation, $gridPageId] = $this->createContextWithGridPage(); + + $this->service->removeNodeFromContextAndPages($relation->getId()); + + $widgets = $this->widgetService->findByPageIds([$gridPageId])[$gridPageId]; + $this->assertCount(2, $widgets); + $this->assertNull($widgets[1]['source']); + $this->assertSame(['pageSize' => 25], $widgets[1]['settings']); + + $this->service->delete($context->getId(), self::OWNER); + + $this->assertSame([], $this->widgetService->findByPageIds([$gridPageId])); + $this->assertSame([], $this->pageMapper->getPageIdsForContext($context->getId())); + } + + /** + * @return array{0: Context, 1: ContextNodeRelation, 2: int} + */ + private function createContextWithGridPage(): array { + $context = $this->service->create('Projects', 'tables', '', [], self::OWNER, Application::OWNER_TYPE_USER); + $this->contextIds[] = $context->getId(); + $relation = $this->service->addNodeToContext($context, 4141, Application::NODE_TYPE_TABLE, Application::PERMISSION_READ); + $this->service->addNodeRelToPage($relation); + + $page = new Page(); + $page->setContextId($context->getId()); + $page->setPageType(Page::TYPE_GRID); + $page->setTitle('Overview'); + $page->setEmoji('📊'); + $page->setDescription('At a glance'); + $page = $this->pageMapper->insert($page); + + $this->widgetService->insert($page->getId(), 'header', 'Welcome', ['gridX' => 0, 'gridY' => 0, 'gridWidth' => 12, 'gridHeight' => 2], ['text' => 'Projects', 'showTitle' => false], null); + $this->widgetService->insert($page->getId(), 'data', null, ['gridX' => 0, 'gridY' => 2, 'gridWidth' => 12, 'gridHeight' => 5], ['pageSize' => 25], $relation->getId()); + + return [$context, $relation, $page->getId()]; + } +} diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index 266827b7cd..6443fe3e05 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -11,11 +11,13 @@ use OCA\Tables\Db\Context; use OCA\Tables\Db\ContextMapper; +use OCA\Tables\Db\ContextNodeRelation; use OCA\Tables\Db\ContextNodeRelationMapper; use OCA\Tables\Db\Page; use OCA\Tables\Db\PageContent; use OCA\Tables\Db\PageContentMapper; use OCA\Tables\Db\PageMapper; +use OCA\Tables\Db\Table; use OCA\Tables\Db\TableMapper; use OCA\Tables\Db\ViewMapper; use OCP\EventDispatcher\IEventDispatcher; @@ -33,6 +35,8 @@ final class ContextServiceTest extends TestCase { private MockObject $contextNodeRelMapper; private MockObject $pageMapper; private MockObject $pageContentMapper; + private MockObject $widgetService; + private MockObject $dbConnection; protected function setUp(): void { parent::setUp(); @@ -45,7 +49,8 @@ protected function setUp(): void { $permissionsService = $this->createMock(PermissionsService::class); $userManager = $this->createMock(IUserManager::class); $eventDispatcher = $this->createMock(IEventDispatcher::class); - $dbConnection = $this->createMock(IDBConnection::class); + $this->dbConnection = $this->createMock(IDBConnection::class); + $this->widgetService = $this->createMock(WidgetService::class); $shareService = $this->createMock(ShareService::class); $navigationManager = $this->createMock(INavigationManager::class); $urlGenerator = $this->createMock(IURLGenerator::class); @@ -59,13 +64,14 @@ protected function setUp(): void { $permissionsService, $userManager, $eventDispatcher, - $dbConnection, + $this->dbConnection, $shareService, false, $navigationManager, $urlGenerator, $this->createMock(TableMapper::class), $this->createMock(ViewMapper::class), + $this->widgetService, ); } @@ -156,6 +162,104 @@ public function testUpdateReordersStartPageContentsFromSubmittedNodes(): void { self::assertSame(10, $updatedContext->getPages()[301]['content'][502]['order']); } + public function testDeleteRemovesTheWidgetsOfEveryPage(): void { + $context = new Context(); + $context->id = 7; + $this->contextMapper->method('findById')->with(7, 'user-1')->willReturn($context); + $this->pageMapper->method('getPageIdsForContext')->with(7)->willReturn([301, 302]); + + $this->widgetService->expects($this->once())->method('deleteByPageIds')->with([301, 302]); + $this->pageMapper->expects($this->exactly(2))->method('deleteByPageId'); + $this->contextMapper->expects($this->once())->method('delete')->with($context); + + $this->service->delete(7, 'user-1'); + } + + public function testDeletingANodeUnlinksItsWidgets(): void { + $this->contextNodeRelMapper->method('getRelIdsForNode')->with(11, 0)->willReturn([101, 102]); + + $this->widgetService->expects($this->once())->method('deleteSourcesByNodeRelIds')->with([101, 102]); + $this->pageContentMapper->expects($this->once())->method('deleteByNodeRelIds')->with([101, 102]); + + $this->service->deleteNodeRel(11, 0); + } + + public function testRemovingANodeFromTheContextUnlinksOnlyItsWidgets(): void { + $relation = new ContextNodeRelation(); + $relation->id = 101; + $this->contextNodeRelMapper->method('findById')->with(101)->willReturn($relation); + $this->contextNodeRelMapper->method('delete')->willReturnArgument(0); + $this->pageContentMapper->method('findByNodeRelation')->willReturn([]); + + $this->widgetService->expects($this->once())->method('deleteSourcesByNodeRelIds')->with([101]); + $this->widgetService->expects($this->never())->method('deleteByPageIds'); + $this->widgetService->expects($this->never())->method('deleteByIds'); + + $this->service->removeNodeFromContextAndPages(101); + } + + public function testGetPagesWithWidgetsAddsTheWidgetsOfEachPage(): void { + $context = new Context(); + $context->setPages([ + 301 => ['id' => 301, 'page_type' => Page::TYPE_STARTPAGE, 'content' => []], + 302 => ['id' => 302, 'page_type' => Page::TYPE_GRID, 'content' => []], + ]); + $widget = ['id' => 9, 'type' => 'text', 'title' => null, 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2, 'settings' => ['text' => 'Hi'], 'source' => null]; + $this->widgetService->method('findByPageIds')->with([301, 302])->willReturn([302 => [$widget]]); + + $pages = $this->service->getPagesWithWidgets($context); + + self::assertSame([], $pages[301]['widgets']); + self::assertSame([$widget], $pages[302]['widgets']); + } + + public function testImportContextRemapsTheSourceOfAWidget(): void { + $table = new Table(); + $table->id = 77; + $table->setOwnership('user-1'); + $this->contextMapper->method('insert')->willReturnCallback(static function (Context $context): Context { + $context->id = 8; + return $context; + }); + $this->contextNodeRelMapper->method('insert')->willReturnCallback(static function (ContextNodeRelation $relation): ContextNodeRelation { + $relation->id = 501; + return $relation; + }); + $insertedPages = []; + $this->pageMapper->method('insert')->willReturnCallback(static function (Page $page) use (&$insertedPages): Page { + $page->id = 600 + count($insertedPages); + $insertedPages[] = $page; + return $page; + }); + + $inserted = []; + $this->widgetService->method('insert')->willReturnCallback(function (...$args) use (&$inserted) { + $inserted[] = $args; + return new \OCA\Tables\Db\Widget(); + }); + + $this->service->importContext($table, [ + 'name' => 'Projects', + 'nodes' => [['id' => 101, 'node_id' => 5, 'node_type' => 0, 'permissions' => 1]], + 'pages' => [[ + 'id' => 301, + 'page_type' => Page::TYPE_GRID, + 'title' => 'Overview', + 'emoji' => '📊', + 'description' => 'At a glance', + 'content' => [], + 'widgets' => [ + ['type' => 'data', 'title' => 'Tasks', 'gridX' => 0, 'gridY' => 2, 'gridWidth' => 12, 'gridHeight' => 5, 'settings' => ['pageSize' => 10], 'source' => ['nodeRelId' => 101]], + ['type' => 'data', 'title' => null, 'gridX' => 0, 'gridY' => 7, 'gridWidth' => 6, 'gridHeight' => 5, 'settings' => [], 'source' => ['nodeRelId' => 999]], + ], + ]], + ], 5); + + self::assertSame(['Overview', '📊', 'At a glance'], [$insertedPages[0]->getTitle(), $insertedPages[0]->getEmoji(), $insertedPages[0]->getDescription()]); + self::assertSame([600, 'data', 'Tasks', ['gridX' => 0, 'gridY' => 2, 'gridWidth' => 12, 'gridHeight' => 5], ['pageSize' => 10], 501], $inserted[0]); + self::assertNull($inserted[1][5]); + } + private function buildPageContent(int $id, int $pageId, int $nodeRelId, int $order): PageContent { $pageContent = new PageContent(); $pageContent->id = $id; diff --git a/tests/unit/Service/WidgetServiceTest.php b/tests/unit/Service/WidgetServiceTest.php new file mode 100644 index 0000000000..94538b914b --- /dev/null +++ b/tests/unit/Service/WidgetServiceTest.php @@ -0,0 +1,139 @@ +settingMapper = new WidgetSettingMapper($this->connectionAdapter); + $this->service = new WidgetService( + new WidgetMapper($this->connectionAdapter), + $this->settingMapper, + new WidgetSourceMapper($this->connectionAdapter), + ); + } + + protected function tearDown(): void { + $this->service->deleteByPageIds([self::PAGE, self::OTHER_PAGE]); + parent::tearDown(); + } + + public function testInsertedWidgetsAreReadBackWithSettingsAndSource(): void { + $data = $this->service->insert(self::PAGE, 'data', 'Tasks', $this->position(0, 4, 12, 5), [], 31); + $header = $this->service->insert(self::PAGE, 'header', null, $this->position(0, 0, 12, 2), [ + 'text' => 'Projects', + 'showTitle' => false, + 'level' => 2, + 'color' => null, + ], null); + $this->service->insert(self::OTHER_PAGE, 'text', null, $this->position(0, 0, 6, 2), ['text' => 'Other'], null); + + $byPage = $this->service->findByPageIds([self::PAGE]); + + $this->assertSame([self::PAGE], array_keys($byPage)); + $this->assertSame([ + [ + 'id' => $header->getId(), + 'type' => 'header', + 'title' => null, + 'gridX' => 0, + 'gridY' => 0, + 'gridWidth' => 12, + 'gridHeight' => 2, + 'settings' => ['text' => 'Projects', 'showTitle' => false, 'level' => 2, 'color' => null], + 'source' => null, + ], + [ + 'id' => $data->getId(), + 'type' => 'data', + 'title' => 'Tasks', + 'gridX' => 0, + 'gridY' => 4, + 'gridWidth' => 12, + 'gridHeight' => 5, + 'settings' => [], + 'source' => ['nodeRelId' => 31], + ], + ], $byPage[self::PAGE]); + } + + public function testRemovingANodeKeepsTheWidgetWithoutSource(): void { + $kept = $this->service->insert(self::PAGE, 'data', null, $this->position(0, 0, 6, 5), [], 41); + $unlinked = $this->service->insert(self::PAGE, 'data', null, $this->position(6, 0, 6, 5), ['pageSize' => 10], 42); + + $this->service->deleteSourcesByNodeRelIds([42]); + + $widgets = $this->service->findByPageIds([self::PAGE])[self::PAGE]; + $this->assertSame([$kept->getId(), $unlinked->getId()], array_column($widgets, 'id')); + $this->assertSame(['nodeRelId' => 41], $widgets[0]['source']); + $this->assertNull($widgets[1]['source']); + $this->assertSame(['pageSize' => 10], $widgets[1]['settings']); + } + + public function testDeletingPagesDeletesWidgetsSettingsAndSources(): void { + $widget = $this->service->insert(self::PAGE, 'data', null, $this->position(0, 0, 6, 5), ['pageSize' => 10], 51); + $other = $this->service->insert(self::OTHER_PAGE, 'data', null, $this->position(0, 0, 6, 5), ['pageSize' => 20], 52); + + $this->service->deleteByPageIds([self::PAGE]); + + $this->assertSame([], $this->service->findByPageIds([self::PAGE])); + $this->assertSame([], $this->settingMapper->findSettingsByWidgetIds([$widget->getId()])); + $this->assertSame([], (new WidgetSourceMapper($this->connectionAdapter))->findNodeRelIdsByWidgetIds([$widget->getId()])); + $this->assertSame(['nodeRelId' => 52], $this->service->findByPageIds([self::OTHER_PAGE])[self::OTHER_PAGE][0]['source']); + $this->assertSame([$other->getId() => ['pageSize' => 20]], $this->settingMapper->findSettingsByWidgetIds([$other->getId()])); + } + + public function testReplacingSettingsDropsSettingsThatAreNotGiven(): void { + $widget = $this->service->insert(self::PAGE, 'header', null, $this->position(0, 0, 12, 2), ['text' => 'A', 'subtitle' => 'B'], null); + + $this->settingMapper->replaceSettings($widget->getId(), ['text' => 'C']); + + $this->assertSame([$widget->getId() => ['text' => 'C']], $this->settingMapper->findSettingsByWidgetIds([$widget->getId()])); + } + + public function testASettingNameIsUniquePerWidget(): void { + $widget = $this->service->insert(self::PAGE, 'header', null, $this->position(0, 0, 12, 2), ['text' => 'A'], null); + $duplicate = new WidgetSetting(); + $duplicate->setWidgetId($widget->getId()); + $duplicate->setName('text'); + $duplicate->setValue('"B"'); + + try { + $this->settingMapper->insert($duplicate); + $this->fail('A second setting with the same name was stored.'); + } catch (Exception $e) { + $this->assertSame(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION, $e->getReason()); + } + } + + /** + * @return array{gridX: int, gridY: int, gridWidth: int, gridHeight: int} + */ + private function position(int $x, int $y, int $width, int $height): array { + return ['gridX' => $x, 'gridY' => $y, 'gridWidth' => $width, 'gridHeight' => $height]; + } +} diff --git a/tests/unit/TablesMigratorTest.php b/tests/unit/TablesMigratorTest.php index c68dcf0198..7de0be2617 100644 --- a/tests/unit/TablesMigratorTest.php +++ b/tests/unit/TablesMigratorTest.php @@ -150,6 +150,35 @@ public function testExportSuccess(): void { $this->assertTrue(true); } + public function testExportWritesTheWidgetsOfEveryContextPage(): void { + $user = $this->createMock(IUser::class); + $exportDestination = $this->createMock(IExportDestination::class); + $user->method('getUID')->willReturn('user1'); + $this->favoritesService->method('findAll')->willReturn([]); + $this->tableMapper->method('findAll')->willReturn([]); + $this->columnMapper->method('findAllByTableIds')->willReturn([]); + $this->rowSleeveMapper->method('findAllByTableIds')->willReturn([]); + $this->viewMapper->method('findAll')->willReturn([]); + $this->shareMapper->method('findAllSharesForTablesAndContexts')->willReturn([]); + + $context = new Context(); + $context->id = 3; + $context->setPages([5 => ['id' => 5, 'page_type' => 'grid', 'content' => []]]); + $this->contextMapper->method('findAll')->willReturn([$context]); + $widget = ['id' => 9, 'type' => 'text', 'settings' => ['text' => 'Hi'], 'source' => null]; + $this->contextService->method('getPagesWithWidgets')->with($context) + ->willReturn([5 => ['id' => 5, 'page_type' => 'grid', 'content' => [], 'widgets' => [$widget]]]); + + $files = []; + $exportDestination->method('addFileContents')->willReturnCallback(static function (string $path, string $content) use (&$files): void { + $files[$path] = $content; + }); + $this->migrator->export($user, $exportDestination, $this->createMock(NullOutput::class)); + + $contexts = json_decode($files['contexts.json'], true); + $this->assertSame([$widget], $contexts[0]['pages'][5]['widgets']); + } + public function testExportFailure(): void { $user = $this->createMock(IUser::class); $exportDestination = $this->createMock(IExportDestination::class); From b66a59e9a1e8f903e2d4a8849e621995055ecd88 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 22 Oct 2026 09:31:43 +0200 Subject: [PATCH 2/3] feat(widgets): store widget settings as one JSON column on the widget Widget settings move from one row per setting in tables_contexts_widget_setting to one JSON object in tables_contexts_widget.settings, as agreed with enjeck on 22 Oct. The rest of the nextcloud/tables#3100 data model stays. The first grid page migration now creates the settings column and no settings table. Version2400Date20261022000001 adds the column on instances that ran the earlier version and copies the setting rows into it; Version2400Date20261022000002 drops the old table afterwards. The app version goes to 2.4.0-dev.9 so these run. Assisted-by: ClaudeCode:claude-opus-5-5 --- appinfo/info.xml | 2 +- lib/Db/Widget.php | 32 +++- lib/Db/WidgetSetting.php | 33 ---- lib/Db/WidgetSettingMapper.php | 88 ----------- .../Version2400Date20261016000001.php | 19 +-- .../Version2400Date20261022000001.php | 96 ++++++++++++ .../Version2400Date20261022000002.php | 35 +++++ lib/Service/WidgetService.php | 22 ++- tests/unit/Db/WidgetTest.php | 42 +++++ .../WidgetSettingsToJsonMigrationTest.php | 145 ++++++++++++++++++ .../Service/ContextPagesRoundTripTest.php | 2 - tests/unit/Service/WidgetServiceTest.php | 45 +++--- 12 files changed, 382 insertions(+), 179 deletions(-) delete mode 100644 lib/Db/WidgetSetting.php delete mode 100644 lib/Db/WidgetSettingMapper.php create mode 100644 lib/Migration/Version2400Date20261022000001.php create mode 100644 lib/Migration/Version2400Date20261022000002.php create mode 100644 tests/unit/Db/WidgetTest.php create mode 100644 tests/unit/Migration/WidgetSettingsToJsonMigrationTest.php diff --git a/appinfo/info.xml b/appinfo/info.xml index 61b25aa33b..abcfa6e312 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -26,7 +26,7 @@ Share your tables and views with users and groups within your cloud. Have a good time and manage whatever you want. ]]> - 2.4.0-dev.5 + 2.4.0-dev.9 AGPL-3.0-or-later Nextcloud GmbH and Nextcloud contributors Tables diff --git a/lib/Db/Widget.php b/lib/Db/Widget.php index ee657cec71..b30759470f 100644 --- a/lib/Db/Widget.php +++ b/lib/Db/Widget.php @@ -12,7 +12,10 @@ use OCP\AppFramework\Db\Entity; /** - * A widget on a grid page: its type, optional title and its place on the grid. + * A widget on a grid page: its type, optional title, its place on the grid and its + * settings, stored as one JSON object. + * + * @psalm-type TablesWidgetSettingValue = string|int|float|bool|null * * @method getPageId(): int * @method setPageId(int $value): void @@ -28,6 +31,8 @@ * @method setGridWidth(int $value): void * @method getGridHeight(): int * @method setGridHeight(int $value): void + * @method getSettings(): ?string + * @method setSettings(?string $value): void */ class Widget extends Entity implements \JsonSerializable { protected ?int $pageId = null; @@ -37,6 +42,7 @@ class Widget extends Entity implements \JsonSerializable { protected ?int $gridY = null; protected ?int $gridWidth = null; protected ?int $gridHeight = null; + protected ?string $settings = null; public function __construct() { $this->addType('id', 'integer'); @@ -47,6 +53,29 @@ public function __construct() { $this->addType('grid_height', 'integer'); } + /** + * @return array + */ + public function getSettingsArray(): array { + $settings = $this->getSettings(); + if ($settings === null || $settings === '') { + return []; + } + $decoded = json_decode($settings, true); + /** @var array */ + return is_array($decoded) ? $decoded : []; + } + + /** + * Stores the settings as one JSON object, or no value when there are none. + * + * @param array $settings + * @throws \JsonException + */ + public function setSettingsArray(array $settings): void { + $this->setSettings($settings === [] ? null : json_encode($settings, JSON_THROW_ON_ERROR)); + } + public function jsonSerialize(): array { return [ 'id' => $this->getId(), @@ -57,6 +86,7 @@ public function jsonSerialize(): array { 'gridY' => $this->getGridY(), 'gridWidth' => $this->getGridWidth(), 'gridHeight' => $this->getGridHeight(), + 'settings' => $this->getSettingsArray(), ]; } } diff --git a/lib/Db/WidgetSetting.php b/lib/Db/WidgetSetting.php deleted file mode 100644 index 983188dec9..0000000000 --- a/lib/Db/WidgetSetting.php +++ /dev/null @@ -1,33 +0,0 @@ -addType('id', 'integer'); - $this->addType('widget_id', 'integer'); - } -} diff --git a/lib/Db/WidgetSettingMapper.php b/lib/Db/WidgetSettingMapper.php deleted file mode 100644 index 7b2c834e1a..0000000000 --- a/lib/Db/WidgetSettingMapper.php +++ /dev/null @@ -1,88 +0,0 @@ - value maps. Values are stored JSON - * encoded, one row per setting. - * - * @psalm-type TablesWidgetSettingValue = string|int|float|bool|null - * @template-extends QBMapper - */ -class WidgetSettingMapper extends QBMapper { - protected string $table = 'tables_contexts_widget_setting'; - - public function __construct(IDBConnection $db) { - parent::__construct($db, $this->table, WidgetSetting::class); - } - - /** - * @param int[] $widgetIds - * @return array> settings by widget id - * @throws Exception - */ - public function findSettingsByWidgetIds(array $widgetIds): array { - if ($widgetIds === []) { - return []; - } - $qb = $this->db->getQueryBuilder(); - $qb->select('widget_id', 'name', 'value') - ->from($this->table) - ->where($qb->expr()->in('widget_id', $qb->createNamedParameter($widgetIds, IQueryBuilder::PARAM_INT_ARRAY))) - ->orderBy('id', 'ASC'); - - $result = $qb->executeQuery(); - $settings = []; - while ($row = $result->fetchAssociative()) { - $settings[(int)$row['widget_id']][(string)$row['name']] = $row['value'] === null - ? null - : json_decode((string)$row['value'], true); - } - $result->closeCursor(); - return $settings; - } - - /** - * Replaces all settings of a widget. - * - * @param array $settings - * @throws Exception - * @throws \JsonException - */ - public function replaceSettings(int $widgetId, array $settings): void { - $this->deleteByWidgetIds([$widgetId]); - foreach ($settings as $name => $value) { - $setting = new WidgetSetting(); - $setting->setWidgetId($widgetId); - $setting->setName((string)$name); - $setting->setValue($value === null ? null : json_encode($value, JSON_THROW_ON_ERROR)); - $this->insert($setting); - } - } - - /** - * @param int[] $widgetIds - * @throws Exception - */ - public function deleteByWidgetIds(array $widgetIds): void { - if ($widgetIds === []) { - return; - } - $qb = $this->db->getQueryBuilder(); - $qb->delete($this->table) - ->where($qb->expr()->in('widget_id', $qb->createNamedParameter($widgetIds, IQueryBuilder::PARAM_INT_ARRAY))) - ->executeStatement(); - } -} diff --git a/lib/Migration/Version2400Date20261016000001.php b/lib/Migration/Version2400Date20261016000001.php index f63f37918b..50f1d08719 100644 --- a/lib/Migration/Version2400Date20261016000001.php +++ b/lib/Migration/Version2400Date20261016000001.php @@ -17,8 +17,8 @@ use Override; /** - * Grid pages: a title, emoji and description on pages, and the widgets of a page - * with their settings and the node a data widget shows. + * Grid pages: a title, emoji and description on pages, the widgets of a page with + * their settings as one JSON object, and the node a data widget shows. */ class Version2400Date20261016000001 extends SimpleMigrationStep { private const PREFIX = 'tables_contexts_'; @@ -30,7 +30,6 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt $this->extendPageTable($schema); $this->haveWidgetTable($schema); - $this->haveWidgetSettingTable($schema); $this->haveWidgetSourceTable($schema); return $schema; @@ -68,23 +67,11 @@ private function haveWidgetTable(ISchemaWrapper $schema): void { $table->addColumn('grid_y', Types::INTEGER, ['notnull' => true, 'default' => 0]); $table->addColumn('grid_width', Types::SMALLINT, ['notnull' => true, 'default' => 1]); $table->addColumn('grid_height', Types::SMALLINT, ['notnull' => true, 'default' => 1]); + $table->addColumn('settings', Types::TEXT, ['notnull' => false]); $table->setPrimaryKey(['id']); $table->addIndex(['page_id'], 'tables_ctx_widget_page_idx'); } - private function haveWidgetSettingTable(ISchemaWrapper $schema): void { - if ($schema->hasTable(self::PREFIX . 'widget_setting')) { - return; - } - $table = $schema->createTable(self::PREFIX . 'widget_setting'); - $table->addColumn('id', Types::INTEGER, ['autoincrement' => true, 'notnull' => true]); - $table->addColumn('widget_id', Types::INTEGER, ['notnull' => true]); - $table->addColumn('name', Types::STRING, ['notnull' => true, 'length' => 64]); - $table->addColumn('value', Types::TEXT, ['notnull' => false]); - $table->setPrimaryKey(['id']); - $table->addUniqueIndex(['widget_id', 'name'], 'tables_ctx_wsetting_uq'); - } - private function haveWidgetSourceTable(ISchemaWrapper $schema): void { if ($schema->hasTable(self::PREFIX . 'widget_source')) { return; diff --git a/lib/Migration/Version2400Date20261022000001.php b/lib/Migration/Version2400Date20261022000001.php new file mode 100644 index 0000000000..a0c57dadbe --- /dev/null +++ b/lib/Migration/Version2400Date20261022000001.php @@ -0,0 +1,96 @@ +hasTable(self::WIDGET_TABLE)) { + return null; + } + $table = $schema->getTable(self::WIDGET_TABLE); + if ($table->hasColumn('settings')) { + return null; + } + $table->addColumn('settings', Types::TEXT, ['notnull' => false]); + return $schema; + } + + #[Override] + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { + if (!$this->connection->tableExists(self::SETTING_TABLE)) { + return; + } + $count = $this->copySettings(); + $output->info('Version2400Date20261022000001: moved the settings of ' . $count . ' widgets into their widget row.'); + } + + /** + * Builds one JSON object per widget from its setting rows, whose values are each + * JSON encoded, and stores it on widgets that have no settings yet. + * + * @return int the number of widgets written + * @throws \JsonException + */ + public function copySettings(): int { + $qb = $this->connection->getQueryBuilder(); + $qb->select('widget_id', 'name', 'value') + ->from(self::SETTING_TABLE) + ->orderBy('widget_id', 'ASC') + ->addOrderBy('id', 'ASC'); + $result = $qb->executeQuery(); + $byWidget = []; + while ($row = $result->fetch()) { + $byWidget[(int)$row['widget_id']][(string)$row['name']] = $row['value'] === null + ? null + : json_decode((string)$row['value'], true); + } + $result->closeCursor(); + + $update = $this->connection->getQueryBuilder(); + $update->update(self::WIDGET_TABLE) + ->set('settings', $update->createParameter('settings')) + ->where($update->expr()->eq('id', $update->createParameter('id'))) + ->andWhere($update->expr()->isNull('settings')); + + $count = 0; + foreach ($byWidget as $widgetId => $settings) { + $update->setParameter('settings', json_encode($settings, JSON_THROW_ON_ERROR)); + $update->setParameter('id', $widgetId, IQueryBuilder::PARAM_INT); + $count += $update->executeStatement(); + } + return $count; + } +} diff --git a/lib/Migration/Version2400Date20261022000002.php b/lib/Migration/Version2400Date20261022000002.php new file mode 100644 index 0000000000..d505521631 --- /dev/null +++ b/lib/Migration/Version2400Date20261022000002.php @@ -0,0 +1,35 @@ +hasTable(Version2400Date20261022000001::SETTING_TABLE)) { + return null; + } + $schema->dropTable(Version2400Date20261022000001::SETTING_TABLE); + return $schema; + } +} diff --git a/lib/Service/WidgetService.php b/lib/Service/WidgetService.php index 8279d6e26b..97e4c5f9a9 100644 --- a/lib/Service/WidgetService.php +++ b/lib/Service/WidgetService.php @@ -11,21 +11,19 @@ use OCA\Tables\Db\Widget; use OCA\Tables\Db\WidgetMapper; -use OCA\Tables\Db\WidgetSettingMapper; use OCA\Tables\Db\WidgetSourceMapper; use OCP\DB\Exception; /** - * Stores the widgets of grid pages together with their settings and the node a - * data widget shows. Callers check permissions and run writes in a transaction. + * Stores the widgets of grid pages, settings included, and the node a data widget + * shows. Callers check permissions and run writes in a transaction. * - * @psalm-import-type TablesWidgetSettingValue from WidgetSettingMapper + * @psalm-import-type TablesWidgetSettingValue from Widget * @psalm-type TablesWidgetRecord = array{id: int, type: string, title: ?string, gridX: int, gridY: int, gridWidth: int, gridHeight: int, settings: array, source: ?array{nodeRelId: int}} */ class WidgetService { public function __construct( private readonly WidgetMapper $widgetMapper, - private readonly WidgetSettingMapper $widgetSettingMapper, private readonly WidgetSourceMapper $widgetSourceMapper, ) { } @@ -38,7 +36,6 @@ public function __construct( public function findByPageIds(array $pageIds): array { $widgets = $this->widgetMapper->findByPageIds($pageIds); $widgetIds = array_map(static fn (Widget $widget): int => $widget->getId(), $widgets); - $settings = $this->widgetSettingMapper->findSettingsByWidgetIds($widgetIds); $sources = $this->widgetSourceMapper->findNodeRelIdsByWidgetIds($widgetIds); $byPage = []; @@ -52,7 +49,7 @@ public function findByPageIds(array $pageIds): array { 'gridY' => $widget->getGridY(), 'gridWidth' => $widget->getGridWidth(), 'gridHeight' => $widget->getGridHeight(), - 'settings' => $settings[$id] ?? [], + 'settings' => $widget->getSettingsArray(), 'source' => isset($sources[$id]) ? ['nodeRelId' => $sources[$id]] : null, ]; } @@ -68,16 +65,15 @@ public function findByPageIds(array $pageIds): array { public function insert(int $pageId, string $type, ?string $title, array $position, array $settings, ?int $nodeRelId): Widget { $widget = new Widget(); $widget->setPageId($pageId); - $this->applyFields($widget, $type, $title, $position); + $this->applyFields($widget, $type, $title, $position, $settings); $widget = $this->widgetMapper->insert($widget); - $this->widgetSettingMapper->replaceSettings($widget->getId(), $settings); $this->widgetSourceMapper->setSource($widget->getId(), $nodeRelId); return $widget; } /** - * Deletes the widgets of the given pages with their settings and sources. + * Deletes the widgets of the given pages with their sources. * * @param int[] $pageIds * @throws Exception @@ -95,7 +91,6 @@ public function deleteByPageIds(array $pageIds): void { * @throws Exception */ public function deleteByIds(array $widgetIds): void { - $this->widgetSettingMapper->deleteByWidgetIds($widgetIds); $this->widgetSourceMapper->deleteByWidgetIds($widgetIds); $this->widgetMapper->deleteByIds($widgetIds); } @@ -112,13 +107,16 @@ public function deleteSourcesByNodeRelIds(array $nodeRelIds): void { /** * @param array{gridX: int, gridY: int, gridWidth: int, gridHeight: int} $position + * @param array $settings + * @throws \JsonException */ - private function applyFields(Widget $widget, string $type, ?string $title, array $position): void { + private function applyFields(Widget $widget, string $type, ?string $title, array $position, array $settings): void { $widget->setType($type); $widget->setTitle($title); $widget->setGridX($position['gridX']); $widget->setGridY($position['gridY']); $widget->setGridWidth($position['gridWidth']); $widget->setGridHeight($position['gridHeight']); + $widget->setSettingsArray($settings); } } diff --git a/tests/unit/Db/WidgetTest.php b/tests/unit/Db/WidgetTest.php new file mode 100644 index 0000000000..ead237ec91 --- /dev/null +++ b/tests/unit/Db/WidgetTest.php @@ -0,0 +1,42 @@ +setSettingsArray(['text' => 'Projects', 'showTitle' => false, 'level' => 2, 'ratio' => 1.5, 'color' => null]); + + $this->assertSame('{"text":"Projects","showTitle":false,"level":2,"ratio":1.5,"color":null}', $widget->getSettings()); + $this->assertSame(['text' => 'Projects', 'showTitle' => false, 'level' => 2, 'ratio' => 1.5, 'color' => null], $widget->getSettingsArray()); + } + + public function testNoSettingsAreStoredAsNoValue(): void { + $widget = new Widget(); + $widget->setSettingsArray([]); + + $this->assertNull($widget->getSettings()); + $this->assertSame([], $widget->getSettingsArray()); + } + + /** + * A stored value that is not a JSON object reads as no settings rather than failing. + */ + public function testAStoredValueThatIsNotAnObjectReadsAsNoSettings(): void { + $widget = new Widget(); + foreach (['', '"text"', '42', 'null', '{broken'] as $stored) { + $widget->setSettings($stored); + $this->assertSame([], $widget->getSettingsArray(), $stored); + } + } +} diff --git a/tests/unit/Migration/WidgetSettingsToJsonMigrationTest.php b/tests/unit/Migration/WidgetSettingsToJsonMigrationTest.php new file mode 100644 index 0000000000..72db2eadc1 --- /dev/null +++ b/tests/unit/Migration/WidgetSettingsToJsonMigrationTest.php @@ -0,0 +1,145 @@ +runUpgrade(); + $qb = $this->connectionAdapter->getQueryBuilder(); + $qb->delete(Version2400Date20261022000001::WIDGET_TABLE) + ->where($qb->expr()->eq('page_id', $qb->createNamedParameter(self::PAGE, IQueryBuilder::PARAM_INT))) + ->executeStatement(); + parent::tearDown(); + } + + public function testSettingRowsBecomeOneJsonObjectAndTheTableIsDropped(): void { + $this->restoreRowShape(); + $header = $this->insertWidget('header'); + $data = $this->insertWidget('data'); + $this->insertSetting($header, 'text', '"Projects"'); + $this->insertSetting($header, 'showTitle', 'false'); + $this->insertSetting($header, 'level', '2'); + $this->insertSetting($header, 'ratio', '1.5'); + $this->insertSetting($header, 'color', null); + + $this->runUpgrade(); + + $expected = '{"text":"Projects","showTitle":false,"level":2,"ratio":1.5,"color":null}'; + $this->assertSame($expected, $this->storedSettings($header)); + $this->assertNull($this->storedSettings($data)); + $this->assertFalse($this->connectionAdapter->tableExists(Version2400Date20261022000001::SETTING_TABLE)); + + $this->runUpgrade(); + + $this->assertSame($expected, $this->storedSettings($header)); + $this->assertNull($this->storedSettings($data)); + $this->assertFalse($this->connectionAdapter->tableExists(Version2400Date20261022000001::SETTING_TABLE)); + } + + public function testAnInstanceWithoutTheSettingTableIsLeftAsItIs(): void { + $widget = $this->insertWidget('text', '{"text":"Kept"}'); + + $this->runUpgrade(); + + $this->assertSame('{"text":"Kept"}', $this->storedSettings($widget)); + $this->assertFalse($this->connectionAdapter->tableExists(Version2400Date20261022000001::SETTING_TABLE)); + } + + /** + * Puts the schema back the way the first grid page migration left it: no settings + * column on the widget, one row per setting in a separate table. + */ + private function restoreRowShape(): void { + $schema = new SchemaWrapper($this->connection); + $schema->getTable(Version2400Date20261022000001::WIDGET_TABLE)->dropColumn('settings'); + $table = $schema->createTable(Version2400Date20261022000001::SETTING_TABLE); + $table->addColumn('id', Types::INTEGER, ['autoincrement' => true, 'notnull' => true]); + $table->addColumn('widget_id', Types::INTEGER, ['notnull' => true]); + $table->addColumn('name', Types::STRING, ['notnull' => true, 'length' => 64]); + $table->addColumn('value', Types::TEXT, ['notnull' => false]); + $table->setPrimaryKey(['id']); + $table->addUniqueIndex(['widget_id', 'name'], 'tables_ctx_wsetting_uq'); + $this->connection->migrateToSchema($schema->getWrappedSchema()); + } + + private function runUpgrade(): void { + $this->runStep(new Version2400Date20261022000001($this->connectionAdapter)); + $this->runStep(new Version2400Date20261022000002()); + } + + /** + * Runs one step the way the migration service does. + */ + private function runStep(SimpleMigrationStep $step): void { + $output = new NullOutput(); + $schema = $step->changeSchema($output, fn (): SchemaWrapper => new SchemaWrapper($this->connection), []); + if ($schema instanceof SchemaWrapper) { + $this->connection->migrateToSchema($schema->getWrappedSchema()); + $schema->performDropTableCalls(); + } + $step->postSchemaChange($output, fn (): SchemaWrapper => new SchemaWrapper($this->connection), []); + } + + private function insertWidget(string $type, ?string $settings = null): int { + $values = [ + 'page_id' => self::PAGE, + 'type' => $type, + 'grid_x' => 0, + 'grid_y' => 0, + 'grid_width' => 1, + 'grid_height' => 1, + ]; + if ($settings !== null) { + $values['settings'] = $settings; + } + $qb = $this->connectionAdapter->getQueryBuilder(); + $qb->insert(Version2400Date20261022000001::WIDGET_TABLE); + foreach ($values as $column => $value) { + $qb->setValue($column, $qb->createNamedParameter($value, is_int($value) ? IQueryBuilder::PARAM_INT : IQueryBuilder::PARAM_STR)); + } + $qb->executeStatement(); + return $qb->getLastInsertId(); + } + + private function insertSetting(int $widgetId, string $name, ?string $value): void { + $qb = $this->connectionAdapter->getQueryBuilder(); + $qb->insert(Version2400Date20261022000001::SETTING_TABLE) + ->setValue('widget_id', $qb->createNamedParameter($widgetId, IQueryBuilder::PARAM_INT)) + ->setValue('name', $qb->createNamedParameter($name)) + ->setValue('value', $qb->createNamedParameter($value, $value === null ? IQueryBuilder::PARAM_NULL : IQueryBuilder::PARAM_STR)) + ->executeStatement(); + } + + private function storedSettings(int $widgetId): ?string { + $qb = $this->connectionAdapter->getQueryBuilder(); + $qb->select('settings') + ->from(Version2400Date20261022000001::WIDGET_TABLE) + ->where($qb->expr()->eq('id', $qb->createNamedParameter($widgetId, IQueryBuilder::PARAM_INT))); + $result = $qb->executeQuery(); + $value = $result->fetchOne(); + $result->closeCursor(); + return $value === false || $value === null ? null : (string)$value; + } +} diff --git a/tests/unit/Service/ContextPagesRoundTripTest.php b/tests/unit/Service/ContextPagesRoundTripTest.php index a7a197efbc..000ec1c5d5 100644 --- a/tests/unit/Service/ContextPagesRoundTripTest.php +++ b/tests/unit/Service/ContextPagesRoundTripTest.php @@ -21,7 +21,6 @@ use OCA\Tables\Db\TableMapper; use OCA\Tables\Db\ViewMapper; use OCA\Tables\Db\WidgetMapper; -use OCA\Tables\Db\WidgetSettingMapper; use OCA\Tables\Db\WidgetSourceMapper; use OCA\Tables\Helper\GroupHelper; use OCA\Tables\Helper\UserHelper; @@ -60,7 +59,6 @@ protected function setUp(): void { $this->pageMapper = new PageMapper($this->connectionAdapter); $this->widgetService = new WidgetService( new WidgetMapper($this->connectionAdapter), - new WidgetSettingMapper($this->connectionAdapter), new WidgetSourceMapper($this->connectionAdapter), ); $this->service = new ContextService( diff --git a/tests/unit/Service/WidgetServiceTest.php b/tests/unit/Service/WidgetServiceTest.php index 94538b914b..050bd591c1 100644 --- a/tests/unit/Service/WidgetServiceTest.php +++ b/tests/unit/Service/WidgetServiceTest.php @@ -10,12 +10,10 @@ namespace OCA\Tables\Tests\Unit\Service; use OCA\Tables\Db\WidgetMapper; -use OCA\Tables\Db\WidgetSetting; -use OCA\Tables\Db\WidgetSettingMapper; use OCA\Tables\Db\WidgetSourceMapper; use OCA\Tables\Service\WidgetService; use OCA\Tables\Tests\Unit\Database\DatabaseTestCase; -use OCP\DB\Exception; +use OCP\DB\QueryBuilder\IQueryBuilder; /** * Runs against the database: widgets, their settings and sources. @@ -25,14 +23,11 @@ class WidgetServiceTest extends DatabaseTestCase { private const OTHER_PAGE = 990002; private WidgetService $service; - private WidgetSettingMapper $settingMapper; protected function setUp(): void { parent::setUp(); - $this->settingMapper = new WidgetSettingMapper($this->connectionAdapter); $this->service = new WidgetService( new WidgetMapper($this->connectionAdapter), - $this->settingMapper, new WidgetSourceMapper($this->connectionAdapter), ); } @@ -101,33 +96,31 @@ public function testDeletingPagesDeletesWidgetsSettingsAndSources(): void { $this->service->deleteByPageIds([self::PAGE]); $this->assertSame([], $this->service->findByPageIds([self::PAGE])); - $this->assertSame([], $this->settingMapper->findSettingsByWidgetIds([$widget->getId()])); + $this->assertNull($this->storedSettings($widget->getId())); $this->assertSame([], (new WidgetSourceMapper($this->connectionAdapter))->findNodeRelIdsByWidgetIds([$widget->getId()])); $this->assertSame(['nodeRelId' => 52], $this->service->findByPageIds([self::OTHER_PAGE])[self::OTHER_PAGE][0]['source']); - $this->assertSame([$other->getId() => ['pageSize' => 20]], $this->settingMapper->findSettingsByWidgetIds([$other->getId()])); + $this->assertSame(['pageSize' => 20], $this->service->findByPageIds([self::OTHER_PAGE])[self::OTHER_PAGE][0]['settings']); + $this->assertSame('{"pageSize":20}', $this->storedSettings($other->getId())); } - public function testReplacingSettingsDropsSettingsThatAreNotGiven(): void { - $widget = $this->service->insert(self::PAGE, 'header', null, $this->position(0, 0, 12, 2), ['text' => 'A', 'subtitle' => 'B'], null); + public function testSettingsAreStoredAsOneJsonObjectOnTheWidgetRow(): void { + $header = $this->service->insert(self::PAGE, 'header', null, $this->position(0, 0, 12, 2), ['text' => 'A', 'level' => 2, 'showTitle' => false, 'color' => null], null); + $data = $this->service->insert(self::PAGE, 'data', null, $this->position(0, 2, 12, 5), [], 61); - $this->settingMapper->replaceSettings($widget->getId(), ['text' => 'C']); - - $this->assertSame([$widget->getId() => ['text' => 'C']], $this->settingMapper->findSettingsByWidgetIds([$widget->getId()])); + $this->assertSame('{"text":"A","level":2,"showTitle":false,"color":null}', $this->storedSettings($header->getId())); + $this->assertNull($this->storedSettings($data->getId())); + $this->assertSame([], $this->service->findByPageIds([self::PAGE])[self::PAGE][1]['settings']); } - public function testASettingNameIsUniquePerWidget(): void { - $widget = $this->service->insert(self::PAGE, 'header', null, $this->position(0, 0, 12, 2), ['text' => 'A'], null); - $duplicate = new WidgetSetting(); - $duplicate->setWidgetId($widget->getId()); - $duplicate->setName('text'); - $duplicate->setValue('"B"'); - - try { - $this->settingMapper->insert($duplicate); - $this->fail('A second setting with the same name was stored.'); - } catch (Exception $e) { - $this->assertSame(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION, $e->getReason()); - } + private function storedSettings(int $widgetId): ?string { + $qb = $this->connectionAdapter->getQueryBuilder(); + $qb->select('settings') + ->from('tables_contexts_widget') + ->where($qb->expr()->eq('id', $qb->createNamedParameter($widgetId, IQueryBuilder::PARAM_INT))); + $result = $qb->executeQuery(); + $value = $result->fetchOne(); + $result->closeCursor(); + return $value === false || $value === null ? null : (string)$value; } /** From e673b07174a7e84421d5a77212866f3539de3baf Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 22 Oct 2026 09:41:25 +0200 Subject: [PATCH 3/3] fix(widgets): store no settings as an empty JSON object An entity setter that receives the value the entity already holds does not mark the field as changed, so saving a widget with no settings over one that had some left the old JSON in place. Writing `{}` makes the update clear them. Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/Db/Widget.php | 4 ++-- lib/Migration/Version2400Date20261022000001.php | 2 +- tests/unit/Db/WidgetTest.php | 8 ++++++-- tests/unit/Service/WidgetServiceTest.php | 2 +- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/Db/Widget.php b/lib/Db/Widget.php index b30759470f..757c8d75ce 100644 --- a/lib/Db/Widget.php +++ b/lib/Db/Widget.php @@ -67,13 +67,13 @@ public function getSettingsArray(): array { } /** - * Stores the settings as one JSON object, or no value when there are none. + * Stores the settings as one JSON object, `{}` when there are none. * * @param array $settings * @throws \JsonException */ public function setSettingsArray(array $settings): void { - $this->setSettings($settings === [] ? null : json_encode($settings, JSON_THROW_ON_ERROR)); + $this->setSettings(json_encode((object)$settings, JSON_THROW_ON_ERROR)); } public function jsonSerialize(): array { diff --git a/lib/Migration/Version2400Date20261022000001.php b/lib/Migration/Version2400Date20261022000001.php index a0c57dadbe..1c8bf56aa5 100644 --- a/lib/Migration/Version2400Date20261022000001.php +++ b/lib/Migration/Version2400Date20261022000001.php @@ -87,7 +87,7 @@ public function copySettings(): int { $count = 0; foreach ($byWidget as $widgetId => $settings) { - $update->setParameter('settings', json_encode($settings, JSON_THROW_ON_ERROR)); + $update->setParameter('settings', json_encode((object)$settings, JSON_THROW_ON_ERROR)); $update->setParameter('id', $widgetId, IQueryBuilder::PARAM_INT); $count += $update->executeStatement(); } diff --git a/tests/unit/Db/WidgetTest.php b/tests/unit/Db/WidgetTest.php index ead237ec91..c2a2e3a650 100644 --- a/tests/unit/Db/WidgetTest.php +++ b/tests/unit/Db/WidgetTest.php @@ -21,14 +21,18 @@ public function testSettingsAreKeptAsOneJsonObject(): void { $this->assertSame(['text' => 'Projects', 'showTitle' => false, 'level' => 2, 'ratio' => 1.5, 'color' => null], $widget->getSettingsArray()); } - public function testNoSettingsAreStoredAsNoValue(): void { + public function testNoSettingsAreStoredAsAnEmptyObject(): void { $widget = new Widget(); $widget->setSettingsArray([]); - $this->assertNull($widget->getSettings()); + $this->assertSame('{}', $widget->getSettings()); $this->assertSame([], $widget->getSettingsArray()); } + public function testNoStoredValueReadsAsNoSettings(): void { + $this->assertSame([], (new Widget())->getSettingsArray()); + } + /** * A stored value that is not a JSON object reads as no settings rather than failing. */ diff --git a/tests/unit/Service/WidgetServiceTest.php b/tests/unit/Service/WidgetServiceTest.php index 050bd591c1..bc14e11854 100644 --- a/tests/unit/Service/WidgetServiceTest.php +++ b/tests/unit/Service/WidgetServiceTest.php @@ -108,7 +108,7 @@ public function testSettingsAreStoredAsOneJsonObjectOnTheWidgetRow(): void { $data = $this->service->insert(self::PAGE, 'data', null, $this->position(0, 2, 12, 5), [], 61); $this->assertSame('{"text":"A","level":2,"showTitle":false,"color":null}', $this->storedSettings($header->getId())); - $this->assertNull($this->storedSettings($data->getId())); + $this->assertSame('{}', $this->storedSettings($data->getId())); $this->assertSame([], $this->service->findByPageIds([self::PAGE])[self::PAGE][1]['settings']); }