From efe01877436fb388c2cae7f13b5b5d2bfe3afa19 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 16 Oct 2026 10:06:21 +0200 Subject: [PATCH] feat(pages): give application pages a technical name Every page gets a required technical name, unique within its application: start pages are "dashboard", new pages get one from their title unless one is given. Existing pages are backfilled. The page API accepts and returns it and answers 400 for an invalid or taken name. Importing a scheme into an application updates the page with the same technical name and adds the others; a user migration import names its pages uniquely. Separate from #3100 so it can be left out upstream. Assisted-by: ClaudeCode:claude-opus-5-5 --- appinfo/info.xml | 2 +- lib/Controller/ContextController.php | 5 +- lib/Controller/ContextPageController.php | 14 +- lib/Db/ContextMapper.php | 3 +- lib/Db/Page.php | 10 ++ lib/Helper/TechnicalNameHelper.php | 100 +++++++++++++ .../Version2400Date20261016000002.php | 120 ++++++++++++++++ .../Version2400Date20261016000003.php | 47 ++++++ lib/ResponseDefinitions.php | 1 + lib/Service/ContextService.php | 134 +++++++++++++++--- lib/Service/PageService.php | 71 +++++++++- openapi.json | 32 ++++- playwright/e2e/context-grid-pages.spec.ts | 25 ++++ src/modules/modals/ContextPageForm.vue | 36 ++++- src/types/openapi/openapi.ts | 22 ++- .../Controller/ContextPageControllerTest.php | 8 +- tests/unit/Db/ContextMapperTest.php | 8 +- tests/unit/Helper/TechnicalNameHelperTest.php | 87 ++++++++++++ .../Version2400Date20261016000002Test.php | 61 ++++++++ .../Service/ContextPagesRoundTripTest.php | 57 +++++++- tests/unit/Service/PageServiceTest.php | 71 +++++++++- 21 files changed, 856 insertions(+), 58 deletions(-) create mode 100644 lib/Helper/TechnicalNameHelper.php create mode 100644 lib/Migration/Version2400Date20261016000002.php create mode 100644 lib/Migration/Version2400Date20261016000003.php create mode 100644 tests/unit/Helper/TechnicalNameHelperTest.php create mode 100644 tests/unit/Migration/Version2400Date20261016000002Test.php diff --git a/appinfo/info.xml b/appinfo/info.xml index 61b25aa33b..5a989beca9 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.6 AGPL-3.0-or-later Nextcloud GmbH and Nextcloud contributors Tables diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index e87f8ca440..39b019a764 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -354,6 +354,7 @@ public function previewSchemeChanges(int $contextId, array $updateScheme): DataR * @param string $description Description of the context * @param list $nodes Ordered meta data of the related nodes * @psalm-param array{addTables: list, modifyTables: list} $tables Tables to be added or modified + * @param list>|null $pages Pages of the scheme; each updates the page with the same technical name or is added, with its widgets * * @return DataResponse, array{}>|DataResponse * @@ -364,10 +365,10 @@ public function previewSchemeChanges(int $contextId, array $updateScheme): DataR */ #[NoAdminRequired] #[RequirePermission(permission: Application::PERMISSION_MANAGE, typeParam: 'context', idParam: 'contextId')] - public function importScheme(int $contextId, string $name, string $iconName, string $description, array $nodes, array $tables): DataResponse { + public function importScheme(int $contextId, string $name, string $iconName, string $description, array $nodes, array $tables, ?array $pages = null): DataResponse { try { $this->db->beginTransaction(); - $context = $this->contextService->importScheme($contextId, $name, $iconName, $description, $nodes, $tables, $this->userId); + $context = $this->contextService->importScheme($contextId, $name, $iconName, $description, $nodes, $tables, $this->userId, $pages === null ? null : array_values($pages)); $this->db->commit(); return new DataResponse($context->jsonSerialize()); } catch (\InvalidArgumentException $e) { diff --git a/lib/Controller/ContextPageController.php b/lib/Controller/ContextPageController.php index ab6e63853c..f860d62933 100644 --- a/lib/Controller/ContextPageController.php +++ b/lib/Controller/ContextPageController.php @@ -72,10 +72,11 @@ public function show(int $contextId, int $pageId): DataResponse { * @param string $title Title of the page * @param string|null $emoji Emoji shown with the title * @param string|null $description Description of the page + * @param string|null $technicalName Name to address the page by, unique within the application; left out makes one from the title * @return DataResponse|DataResponse * * 200: Page created - * 400: Invalid title, emoji or description + * 400: Invalid title, emoji, description or technical name * 403: No permissions * 404: Not found * @@ -83,9 +84,9 @@ public function show(int $contextId, int $pageId): DataResponse { */ #[NoAdminRequired] #[RequirePermission(permission: Application::PERMISSION_MANAGE, typeParam: 'context', idParam: 'contextId')] - public function create(int $contextId, string $title, ?string $emoji = null, ?string $description = null): DataResponse { + public function create(int $contextId, string $title, ?string $emoji = null, ?string $description = null, ?string $technicalName = null): DataResponse { try { - return new DataResponse($this->pageService->create($contextId, $title, $emoji, $description, $this->userId)); + return new DataResponse($this->pageService->create($contextId, $title, $emoji, $description, $technicalName, $this->userId)); } catch (BadRequestError $e) { return $this->handleBadRequestError($e); } catch (PermissionError $e) { @@ -117,10 +118,11 @@ public function create(int $contextId, string $title, ?string $emoji = null, ?st * @param string|null $emoji New emoji, empty to remove it * @param string|null $description New description, empty to remove it * @param list>|null $widgets All widgets of the page + * @param string|null $technicalName New technical name, unique within the application * @return DataResponse|DataResponse * * 200: Page updated - * 400: Invalid page field or widget + * 400: Invalid page field, technical name or widget * 403: No permissions on the application or on a table or view to add * 404: Not found * @@ -128,9 +130,9 @@ public function create(int $contextId, string $title, ?string $emoji = null, ?st */ #[NoAdminRequired] #[RequirePermission(permission: Application::PERMISSION_MANAGE, typeParam: 'context', idParam: 'contextId')] - public function update(int $contextId, int $pageId, ?string $title = null, ?string $emoji = null, ?string $description = null, ?array $widgets = null): DataResponse { + public function update(int $contextId, int $pageId, ?string $title = null, ?string $emoji = null, ?string $description = null, ?array $widgets = null, ?string $technicalName = null): DataResponse { try { - return new DataResponse($this->pageService->update($contextId, $pageId, $title, $emoji, $description, $widgets, $this->userId)); + return new DataResponse($this->pageService->update($contextId, $pageId, $title, $emoji, $description, $widgets, $this->userId, $technicalName)); } catch (BadRequestError $e) { return $this->handleBadRequestError($e); } catch (PermissionError $e) { diff --git a/lib/Db/ContextMapper.php b/lib/Db/ContextMapper.php index 71e13a5ff4..301fd66ead 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.title as page_title', 'p.emoji as page_emoji', 'p.description as page_description', + 'p.id as page_id', 'p.page_type', 'p.title as page_title', 'p.emoji as page_emoji', 'p.description as page_description', 'p.technical_name as page_technical_name', 'pc.id as content_id', 'pc.order', 'n.display_mode as display_mode_default', 's.id as share_id', 's.receiver', 's.receiver_type' @@ -134,6 +134,7 @@ protected function formatResultRows(array $rows, ?string $userId) { $carry[$item['page_id']]['title'] = $item['page_title']; $carry[$item['page_id']]['emoji'] = $item['page_emoji']; $carry[$item['page_id']]['description'] = $item['page_description']; + $carry[$item['page_id']]['technical_name'] = $item['page_technical_name']; if ($item['node_rel_id'] !== null && $item['content_id'] !== null) { $carry[$item['page_id']]['content'][$item['content_id']] = [ 'order' => (int)$item['order'], diff --git a/lib/Db/Page.php b/lib/Db/Page.php index 5e18089da3..f623446ebf 100644 --- a/lib/Db/Page.php +++ b/lib/Db/Page.php @@ -8,6 +8,7 @@ namespace OCA\Tables\Db; +use OCA\Tables\Helper\TechnicalNameHelper; use OCP\AppFramework\Db\Entity; /** @@ -21,21 +22,29 @@ * @method setEmoji(?string $value): void * @method getDescription(): ?string * @method setDescription(?string $value): void + * @method getTechnicalName(): ?string + * @method setTechnicalName(?string $value): void */ class Page extends Entity implements \JsonSerializable { public const TYPE_STARTPAGE = 'startpage'; public const TYPE_GRID = 'grid'; + public const TECHNICAL_NAME_MAX_LENGTH = TechnicalNameHelper::MAX_LENGTH; protected ?int $contextId = null; protected ?string $pageType = null; protected ?string $title = null; protected ?string $emoji = null; protected ?string $description = null; + protected ?string $technicalName = null; public function __construct() { $this->addType('id', 'integer'); } + public static function isValidTechnicalName(string $technicalName): bool { + return TechnicalNameHelper::isValid($technicalName); + } + public function jsonSerialize(): array { return [ 'id' => $this->getId(), @@ -44,6 +53,7 @@ public function jsonSerialize(): array { 'title' => $this->getTitle(), 'emoji' => $this->getEmoji(), 'description' => $this->getDescription(), + 'technicalName' => $this->getTechnicalName(), ]; } } diff --git a/lib/Helper/TechnicalNameHelper.php b/lib/Helper/TechnicalNameHelper.php new file mode 100644 index 0000000000..e7e227977a --- /dev/null +++ b/lib/Helper/TechnicalNameHelper.php @@ -0,0 +1,100 @@ + $taken + */ + public static function unique(string $base, array $taken, int $maxLength = self::MAX_LENGTH): string { + $technicalName = $base; + for ($suffix = 2; isset($taken[$technicalName]); $suffix++) { + $ending = '_' . $suffix; + $technicalName = rtrim(substr($base, 0, $maxLength - strlen($ending)), '_') . $ending; + } + return $technicalName; + } + + /** + * Names a set of pages within one application. Pages whose given name is free + * keep it, in the order given; every other page gets a unique name made from its + * given name, else its fallback. + * + * @param array $pages + * @param array $taken names already used in the application + * @return array the name of every page, by its key + */ + public static function assign(array $pages, array $taken = []): array { + $names = []; + foreach ($pages as $key => $page) { + $given = $page['name']; + if ($given !== null && $given !== '' && self::isValid($given) && !isset($taken[$given])) { + $names[$key] = $given; + $taken[$given] = true; + } + } + foreach ($pages as $key => $page) { + if (isset($names[$key])) { + continue; + } + $given = $page['name']; + $base = $given !== null && $given !== '' ? self::fromText($given) : $page['fallback']; + $names[$key] = self::unique($base, $taken); + $taken[$names[$key]] = true; + } + $ordered = []; + foreach (array_keys($pages) as $key) { + $ordered[$key] = $names[$key]; + } + return $ordered; + } + + /** + * The name a page gets when none is given: `dashboard` for a start page, else + * one made from its title. + */ + public static function pageFallback(bool $isStartPage, ?string $title): string { + if ($isStartPage) { + return self::START_PAGE_NAME; + } + return self::fromText($title ?? ''); + } + + public static function isValid(string $technicalName, int $maxLength = self::MAX_LENGTH): bool { + return strlen($technicalName) <= $maxLength + && preg_match(self::PATTERN, $technicalName) === 1; + } +} diff --git a/lib/Migration/Version2400Date20261016000002.php b/lib/Migration/Version2400Date20261016000002.php new file mode 100644 index 0000000000..471ee6d21d --- /dev/null +++ b/lib/Migration/Version2400Date20261016000002.php @@ -0,0 +1,120 @@ +`. Valid names that are free keep their value. + */ +class Version2400Date20261016000002 extends SimpleMigrationStep { + private const TABLE = 'tables_contexts_page'; + + public function __construct( + private readonly IDBConnection $db, + ) { + } + + #[Override] + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + if (!$schema->hasTable(self::TABLE)) { + return null; + } + $table = $schema->getTable(self::TABLE); + if ($table->hasColumn('technical_name')) { + return null; + } + $table->addColumn('technical_name', Types::STRING, ['notnull' => false, 'length' => TechnicalNameHelper::MAX_LENGTH]); + return $schema; + } + + /** + * @param Closure(): ISchemaWrapper $schemaClosure + */ + #[Override] + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { + $select = $this->db->getQueryBuilder(); + $select->select('id', 'context_id', 'page_type', 'title', 'technical_name') + ->from(self::TABLE) + ->orderBy('context_id') + ->addOrderBy('id'); + $result = $select->executeQuery(); + $rows = $result->fetchAll(); + $result->closeCursor(); + + $names = self::backfillNames($rows); + if ($names === []) { + return; + } + + $update = $this->db->getQueryBuilder(); + $update->update(self::TABLE) + ->set('technical_name', $update->createParameter('technicalName')) + ->where($update->expr()->eq('id', $update->createParameter('pageId'))); + + $this->db->beginTransaction(); + try { + foreach ($names as $pageId => $technicalName) { + $update->setParameter('technicalName', $technicalName); + $update->setParameter('pageId', $pageId, IQueryBuilder::PARAM_INT); + $update->executeStatement(); + } + $this->db->commit(); + } catch (\Exception $e) { + $this->db->rollBack(); + throw $e; + } + } + + /** + * The new technical name of every page row that needs one, by page id. + * + * @param list> $rows page rows ordered by id + * @return array + */ + public static function backfillNames(array $rows): array { + $pagesByContext = []; + foreach ($rows as $row) { + $pageId = (int)$row['id']; + $title = is_string($row['title'] ?? null) ? $row['title'] : ''; + $isStartPage = $row['page_type'] === Page::TYPE_STARTPAGE; + $fallback = $isStartPage || preg_match('/[a-z0-9]/i', $title) === 1 + ? TechnicalNameHelper::pageFallback($isStartPage, $title) + : 'page_' . $pageId; + $pagesByContext[(int)$row['context_id']][$pageId] = [ + 'name' => is_string($row['technical_name'] ?? null) ? $row['technical_name'] : null, + 'fallback' => $fallback, + ]; + } + + $changes = []; + foreach ($pagesByContext as $pages) { + foreach (TechnicalNameHelper::assign($pages) as $pageId => $technicalName) { + if ($pages[$pageId]['name'] !== $technicalName) { + $changes[$pageId] = $technicalName; + } + } + } + return $changes; + } +} diff --git a/lib/Migration/Version2400Date20261016000003.php b/lib/Migration/Version2400Date20261016000003.php new file mode 100644 index 0000000000..579779bcd5 --- /dev/null +++ b/lib/Migration/Version2400Date20261016000003.php @@ -0,0 +1,47 @@ +hasTable('tables_contexts_page')) { + return null; + } + + $table = $schema->getTable('tables_contexts_page'); + $changed = false; + $technicalName = $table->getColumn('technical_name'); + if (!$technicalName->getNotnull()) { + $technicalName->setNotnull(true); + $changed = true; + } + if (!$table->hasIndex(self::INDEX)) { + $table->addUniqueIndex(['context_id', 'technical_name'], self::INDEX); + $changed = true; + } + + return $changed ? $schema : null; + } +} diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index e78044cc2a..8648bb0ab9 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -275,6 +275,7 @@ * title: ?string, * emoji: ?string, * description: ?string, + * technicalName: string, * widgets: list, * } * diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 78e806e429..98da958e3e 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -25,6 +25,7 @@ use OCA\Tables\Errors\InternalError; use OCA\Tables\Errors\NotFoundError; use OCA\Tables\Errors\PermissionError; +use OCA\Tables\Helper\TechnicalNameHelper; use OCA\Tables\Model\ColumnSettings; use OCA\Tables\Model\ContextScheme; use OCA\Tables\Model\FilterSet; @@ -530,6 +531,7 @@ protected function insertPage(Context $context): void { $page = new Page(); $page->setContextId($context->getId()); $page->setPageType(Page::TYPE_STARTPAGE); + $page->setTechnicalName(TechnicalNameHelper::START_PAGE_NAME); $this->pageMapper->insert($page); $addedPage = $page->jsonSerialize(); @@ -635,14 +637,20 @@ public function importContext(Table $table, array $context, int $tableOldId): ?C // insert pages $pageMap = []; + $relIdMap = array_map(static fn (ContextNodeRelation $relation): int => $relation->getId(), $nodeRelMap); if (!empty($context['pages'])) { - foreach ($context['pages'] as $pageArr) { + $technicalNames = TechnicalNameHelper::assign(array_map(static fn (array $pageArr): array => [ + 'name' => is_string($pageArr['technical_name'] ?? null) ? $pageArr['technical_name'] : null, + 'fallback' => TechnicalNameHelper::pageFallback(($pageArr['page_type'] ?? $pageArr['pageType'] ?? '') === Page::TYPE_STARTPAGE, $pageArr['title'] ?? null), + ], $context['pages'])); + foreach ($context['pages'] as $pageKey => $pageArr) { $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->setTechnicalName($technicalNames[$pageKey]); $page = $this->pageMapper->insert($page); $pageMap[$pageArr['id'] ?? null] = $page; @@ -660,7 +668,10 @@ public function importContext(Table $table, array $context, int $tableOldId): ?C } foreach ($pageArr['widgets'] ?? [] as $widgetArr) { - $this->importWidget($page->getId(), $widgetArr, $nodeRelMap); + $widget = $this->readImportedWidget($widgetArr, $relIdMap); + if ($widget !== null) { + $this->widgetService->insert($page->getId(), $widget['type'], $widget['title'], $widget['position'], $widget['settings'], $widget['nodeRelId']); + } } } } @@ -683,40 +694,117 @@ public function getPagesWithWidgets(Context $context): array { } /** - * Inserts an exported widget on a page. Its source is remapped to the imported node - * relation, and dropped when that node was not imported. A widget whose type or - * settings are not valid is skipped. + * An exported widget as it is stored again. Its source is remapped to the imported + * node relation and dropped when that node was not imported. A widget whose type or + * settings are not valid gives null. * - * @param array $nodeRelMap imported node relations by their exported id - * @throws Exception - * @throws \JsonException + * @param array $relIdMap imported node relation ids by their exported id + * @return array{id: null, type: string, title: ?string, position: array{gridX: int, gridY: int, gridWidth: int, gridHeight: int}, settings: array, nodeRelId: ?int}|null */ - private function importWidget(int $pageId, mixed $widgetArr, array $nodeRelMap): void { + private function readImportedWidget(mixed $widgetArr, array $relIdMap): ?array { if (!is_array($widgetArr) || !is_string($widgetArr['type'] ?? null)) { - return; + return null; } try { $hasSource = $this->gridWidgetTypes->get($widgetArr['type'])['hasSource']; $settings = $this->gridWidgetTypes->sanitizeSettings($widgetArr['type'], $widgetArr['settings'] ?? null); } catch (BadRequestError $e) { $this->logger->warning('Skipping an imported widget: ' . $e->getMessage(), ['exception' => $e]); - return; + return null; } $oldNodeRelId = $hasSource ? ($widgetArr['source']['nodeRelId'] ?? null) : null; - $newNodeRel = $oldNodeRelId !== null ? ($nodeRelMap[$oldNodeRelId] ?? null) : null; - $this->widgetService->insert( - $pageId, - $widgetArr['type'], - is_string($widgetArr['title'] ?? null) ? $widgetArr['title'] : null, - [ + return [ + 'id' => null, + 'type' => $widgetArr['type'], + 'title' => is_string($widgetArr['title'] ?? null) ? $widgetArr['title'] : null, + 'position' => [ '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(), - ); + 'settings' => $settings, + 'nodeRelId' => $oldNodeRelId !== null ? ($relIdMap[$oldNodeRelId] ?? null) : null, + ]; + } + + /** + * Applies the pages of a scheme to an application. A scheme page updates the page of + * the application with the same technical name, or is added as a new page; its + * widgets replace the widgets of that page. Pages the scheme does not name stay. + * + * @param list $schemeNodes the nodes of the scheme with their exported relation id + * @param array $schemePages + * @throws Exception + * @throws NotFoundError + * @throws \JsonException + */ + public function importSchemePages(int $contextId, array $schemeNodes, array $schemePages): void { + $context = $this->contextMapper->findById($contextId); + $relIdsByNode = []; + foreach ($context->getNodes() ?? [] as $relId => $node) { + $relIdsByNode[sprintf('t%di%d', $node['node_type'], $node['node_id'])] = (int)$relId; + } + $relIdMap = []; + foreach ($schemeNodes as $node) { + if (!isset($node['id'], $node['node_uuid'])) { + continue; + } + try { + $nodeId = $node['node_type'] === Application::NODE_TYPE_VIEW + ? $this->viewMapper->findByUuid($node['node_uuid'])->getId() + : $this->tableMapper->findByUuid($node['node_uuid'])->getId(); + } catch (DoesNotExistException|MultipleObjectsReturnedException) { + continue; + } + $relId = $relIdsByNode[sprintf('t%di%d', $node['node_type'], $nodeId)] ?? null; + if ($relId !== null) { + $relIdMap[$node['id']] = $relId; + } + } + + $pageIdsByName = []; + foreach ($context->getPages() ?? [] as $pageId => $page) { + if (is_string($page['technical_name'] ?? null)) { + $pageIdsByName[$page['technical_name']] = (int)$pageId; + } + } + + foreach ($schemePages as $pageArr) { + if (!is_array($pageArr)) { + continue; + } + $isStartPage = ($pageArr['page_type'] ?? $pageArr['pageType'] ?? '') === Page::TYPE_STARTPAGE; + $name = $pageArr['technical_name'] ?? null; + $name = is_string($name) && Page::isValidTechnicalName($name) + ? $name + : TechnicalNameHelper::pageFallback($isStartPage, $pageArr['title'] ?? null); + + if (isset($pageIdsByName[$name])) { + $page = $this->pageMapper->findById($pageIdsByName[$name]); + } elseif ($isStartPage) { + continue; + } else { + $page = new Page(); + $page->setContextId($contextId); + $page->setPageType(Page::TYPE_GRID); + $page->setTechnicalName($name); + } + $page->setTitle(is_string($pageArr['title'] ?? null) ? $pageArr['title'] : null); + $page->setEmoji(is_string($pageArr['emoji'] ?? null) ? $pageArr['emoji'] : null); + $page->setDescription(is_string($pageArr['description'] ?? null) ? $pageArr['description'] : null); + $page = $page->getId() === null ? $this->pageMapper->insert($page) : $this->pageMapper->update($page); + $pageIdsByName[$name] = $page->getId(); + + $widgets = []; + foreach ($pageArr['widgets'] ?? [] as $widgetArr) { + $widget = $this->readImportedWidget($widgetArr, $relIdMap); + if ($widget !== null) { + $widgets[] = $widget; + } + } + $this->widgetService->syncPage($page->getId(), $widgets); + } } /** @@ -863,7 +951,7 @@ public function compareSchemeChanges(int $contextId, array $updateScheme, string * @throws NotFoundExceptionInterface * @throws PermissionError */ - public function importScheme(int $contextId, string $name, string $iconName, string $description, array $nodes, array $tables, string $userId): Context { + public function importScheme(int $contextId, string $name, string $iconName, string $description, array $nodes, array $tables, string $userId, ?array $pages = null): Context { // Validate the structure of the columns and views arrays if (!isset($tables['addTables']) || !is_array($tables['addTables']) || !isset($tables['modifyTables']) || !is_array($tables['modifyTables'])) { @@ -924,6 +1012,10 @@ public function importScheme(int $contextId, string $name, string $iconName, str } $context = $this->update($contextId, $userId, $name, $iconName, $description, $resolvedNodes); + if ($pages !== null) { + $this->importSchemePages($contextId, $nodes, $pages); + $context = $this->contextMapper->findById($contextId, $userId); + } return $context; } diff --git a/lib/Service/PageService.php b/lib/Service/PageService.php index 661de34643..b08dbdfc68 100644 --- a/lib/Service/PageService.php +++ b/lib/Service/PageService.php @@ -18,6 +18,7 @@ use OCA\Tables\Errors\BadRequestError; use OCA\Tables\Errors\NotFoundError; use OCA\Tables\Errors\PermissionError; +use OCA\Tables\Helper\TechnicalNameHelper; use OCA\Tables\ResponseDefinitions; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; @@ -72,12 +73,14 @@ public function find(int $contextId, int $pageId, string $userId): array { 'title' => $page['title'] ?? null, 'emoji' => $page['emoji'] ?? null, 'description' => $page['description'] ?? null, + 'technicalName' => (string)($page['technical_name'] ?? ''), 'widgets' => $widgets[$pageId] ?? [], ]; } /** - * Adds a grid page to an application. + * Adds a grid page to an application. Without a technical name the page gets one + * made from its title. * * @return TablesPage * @throws BadRequestError @@ -85,8 +88,8 @@ public function find(int $contextId, int $pageId, string $userId): array { * @throws PermissionError * @throws Exception */ - public function create(int $contextId, string $title, ?string $emoji, ?string $description, string $userId): array { - $this->getManageableContext($contextId, $userId); + public function create(int $contextId, string $title, ?string $emoji, ?string $description, ?string $technicalName, string $userId): array { + $context = $this->getManageableContext($contextId, $userId); $page = new Page(); $page->setContextId($contextId); @@ -94,7 +97,14 @@ public function create(int $contextId, string $title, ?string $emoji, ?string $d $page->setTitle($this->validateTitle($title)); $page->setEmoji($this->validateEmoji($emoji)); $page->setDescription($this->validateDescription($description)); - $page = $this->pageMapper->insert($page); + $page->setTechnicalName($technicalName === null + ? TechnicalNameHelper::unique(TechnicalNameHelper::fromText($page->getTitle() ?? ''), $this->takenTechnicalNames($context, null)) + : $this->validateTechnicalName($context, $technicalName, null)); + try { + $page = $this->pageMapper->insert($page); + } catch (Exception $e) { + throw $this->technicalNameTaken($e, (string)$page->getTechnicalName()); + } return $this->find($contextId, $page->getId(), $userId); } @@ -119,7 +129,7 @@ public function create(int $contextId, string $title, ?string $emoji, ?string $d * @throws PermissionError * @throws Exception */ - public function update(int $contextId, int $pageId, ?string $title, ?string $emoji, ?string $description, ?array $widgets, string $userId): array { + public function update(int $contextId, int $pageId, ?string $title, ?string $emoji, ?string $description, ?array $widgets, string $userId, ?string $technicalName = null): array { $context = $this->getManageableContext($contextId, $userId); $pageArray = $this->getPageArray($context, $pageId); @@ -133,6 +143,9 @@ public function update(int $contextId, int $pageId, ?string $title, ?string $emo if ($description !== null) { $page->setDescription($this->validateDescription($description)); } + if ($technicalName !== null) { + $page->setTechnicalName($this->validateTechnicalName($context, $technicalName, $pageId)); + } $rows = null; $nodesToAdd = []; @@ -145,7 +158,11 @@ public function update(int $contextId, int $pageId, ?string $title, ?string $emo $this->atomic(function () use ($page, $rows, $nodesToAdd, $contextId, $pageId, $userId): void { if ($page->getUpdatedFields() !== []) { - $this->pageMapper->update($page); + try { + $this->pageMapper->update($page); + } catch (Exception $e) { + throw $this->technicalNameTaken($e, (string)$page->getTechnicalName()); + } } if ($rows === null) { return; @@ -328,6 +345,48 @@ private function validateTitle(string $title): string { return $title; } + /** + * A technical name is unique within its application. + * + * @throws BadRequestError + */ + private function validateTechnicalName(Context $context, string $technicalName, ?int $pageId): string { + if (!Page::isValidTechnicalName($technicalName)) { + throw new BadRequestError('A technical name has 1 to ' . Page::TECHNICAL_NAME_MAX_LENGTH . ' characters, starts with a lowercase letter and holds only lowercase letters, digits and underscores.'); + } + if (isset($this->takenTechnicalNames($context, $pageId)[$technicalName])) { + throw new BadRequestError('Another page of this application already uses the technical name ' . $technicalName . '.'); + } + return $technicalName; + } + + /** + * A write that another request beat to the same technical name is a bad request; + * any other database error is passed on. + */ + private function technicalNameTaken(Exception $e, string $technicalName): BadRequestError|Exception { + if ($e->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + return new BadRequestError('Another page of this application already uses the technical name ' . $technicalName . '.', 0, $e); + } + return $e; + } + + /** + * The technical names of the application's pages, apart from the given page. + * + * @return array + */ + private function takenTechnicalNames(Context $context, ?int $pageId): array { + $taken = []; + foreach ($context->getPages() ?? [] as $id => $page) { + $technicalName = $page['technical_name'] ?? null; + if ((int)$id !== $pageId && is_string($technicalName)) { + $taken[$technicalName] = true; + } + } + return $taken; + } + /** * @throws BadRequestError */ diff --git a/openapi.json b/openapi.json index 4e26f99769..ac99d86057 100644 --- a/openapi.json +++ b/openapi.json @@ -460,6 +460,7 @@ "title", "emoji", "description", + "technicalName", "widgets" ], "properties": { @@ -486,6 +487,9 @@ "type": "string", "nullable": true }, + "technicalName": { + "type": "string" + }, "widgets": { "type": "array", "items": { @@ -13678,6 +13682,18 @@ } } } + }, + "pages": { + "type": "array", + "nullable": true, + "default": null, + "description": "Pages of the scheme; each updates the page with the same technical name or is added, with its widgets", + "items": { + "type": "object", + "additionalProperties": { + "type": "object" + } + } } } } @@ -14729,6 +14745,12 @@ "type": "object" } } + }, + "technicalName": { + "type": "string", + "nullable": true, + "default": null, + "description": "New technical name, unique within the application" } } } @@ -14799,7 +14821,7 @@ } }, "400": { - "description": "Invalid page field or widget", + "description": "Invalid page field, technical name or widget", "content": { "application/json": { "schema": { @@ -15281,6 +15303,12 @@ "nullable": true, "default": null, "description": "Description of the page" + }, + "technicalName": { + "type": "string", + "nullable": true, + "default": null, + "description": "Name to address the page by, unique within the application; left out makes one from the title" } } } @@ -15341,7 +15369,7 @@ } }, "400": { - "description": "Invalid title, emoji or description", + "description": "Invalid title, emoji, description or technical name", "content": { "application/json": { "schema": { diff --git a/playwright/e2e/context-grid-pages.spec.ts b/playwright/e2e/context-grid-pages.spec.ts index d96dc93daa..684eddbad8 100644 --- a/playwright/e2e/context-grid-pages.spec.ts +++ b/playwright/e2e/context-grid-pages.spec.ts @@ -179,6 +179,31 @@ test.describe('Grid pages of an application', () => { await shareeContext.close() }) + test('A page gets a technical name from its title, and a taken one is refused', async ({ userPage: { page, user } }) => { + const table = await createTableViaApi(page.request, user, 'grid name table') + const contextId = await createApplicationViaApi(page.request, user, 'grid name app', [table]) + await openApplication(page, contextId) + + await page.locator('[data-cy="contextPageAdd"]').click() + await page.locator('[data-cy="contextPageForm"]').getByRole('textbox', { name: 'Title' }).fill('Open cases') + await page.locator('[data-cy="contextPageSubmit"]').click() + await expect(page).toHaveURL(/\/application\/\d+\/page\/\d+/) + + const pages = Object.values((await getApplication(page.request, user, contextId)).pages as Record) + expect(pages.map(entry => entry.technical_name).sort()).toEqual(['dashboard', 'open_cases']) + + await page.locator('[data-cy="gridPageActions"] button').first().click() + await page.locator('[data-cy="gridPageEditDetails"]').click() + const technicalName = page.locator('[data-cy="contextPageForm"]').getByRole('textbox', { name: 'Technical name' }) + await expect(technicalName).toHaveValue('open_cases') + await technicalName.fill('') + await expect(page.locator('[data-cy="contextPageSubmit"]')).toBeDisabled() + await technicalName.fill('dashboard') + const renamed = page.waitForResponse(r => /\/pages\/\d+/.test(r.url()) && r.request().method() === 'PATCH') + await page.locator('[data-cy="contextPageSubmit"]').click() + expect((await renamed).status()).toBe(400) + }) + test('A table removed from the application leaves its widget asking for a new one', async ({ userPage: { page, user } }) => { const table = await createTableViaApi(page.request, user, 'grid removed table') const contextId = await createApplicationViaApi(page.request, user, 'grid removal app', [table]) diff --git a/src/modules/modals/ContextPageForm.vue b/src/modules/modals/ContextPageForm.vue index ec1f829885..12572c1dae 100644 --- a/src/modules/modals/ContextPageForm.vue +++ b/src/modules/modals/ContextPageForm.vue @@ -30,6 +30,12 @@ {{ t('tables', 'Remove emoji') }} + import { NcButton, NcDialog, NcEmojiPicker, NcTextArea, NcTextField } from '@nextcloud/vue' +import { translate as t } from '@nextcloud/l10n' import EmoticonOutline from 'vue-material-design-icons/EmoticonOutline.vue' +const TECHNICAL_NAME = /^[a-z][a-z0-9_]*$/ + /** - * Asks for the title, emoji and description of a new grid page, or of the page being edited. + * Asks for the title, emoji, technical name and description of a new grid page, or of + * the page being edited. */ export default { name: 'ContextPageForm', @@ -78,12 +88,27 @@ export default { return { title: '', emoji: '', + technicalName: '', description: '', } }, computed: { + // A new page may leave it empty, the server then makes one from the title + technicalNameIsValid() { + return (this.technicalName === '' && !this.page) || TECHNICAL_NAME.test(this.technicalName) + }, + technicalNameHelperText() { + if (!this.technicalNameIsValid) { + return this.technicalName === '' + ? t('tables', 'Every page needs a technical name.') + : t('tables', 'Start with a lowercase letter and use only lowercase letters, digits and underscores.') + } + return this.page + ? t('tables', 'Lowercase letters, digits and underscores, starting with a letter.') + : t('tables', 'Leave empty to make one from the title.') + }, canSubmit() { - return this.title.trim() !== '' + return this.title.trim() !== '' && this.technicalNameIsValid }, }, watch: { @@ -91,6 +116,7 @@ export default { if (open) { this.title = this.page?.title ?? '' this.emoji = this.page?.emoji ?? '' + this.technicalName = this.page?.technicalName ?? '' this.description = this.page?.description ?? '' } }, @@ -100,7 +126,11 @@ export default { if (!this.canSubmit) { return } - this.$emit('submit', { title: this.title.trim(), emoji: this.emoji, description: this.description.trim() }) + const data = { title: this.title.trim(), emoji: this.emoji, description: this.description.trim() } + if (this.technicalName !== '') { + data.technicalName = this.technicalName + } + this.$emit('submit', data) }, }, } diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 57781b5a03..675e772bb5 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1254,6 +1254,7 @@ export type components = { readonly title: string | null; readonly emoji: string | null; readonly description: string | null; + readonly technicalName: string; readonly widgets: readonly components["schemas"]["Widget"][]; }; readonly PublicColumn: { @@ -7629,6 +7630,13 @@ export interface operations { readonly addTables: readonly components["schemas"]["Table"][]; readonly modifyTables: readonly components["schemas"]["Table"][]; }; + /** + * @description Pages of the scheme; each updates the page with the same technical name or is added, with its widgets + * @default null + */ + readonly pages?: readonly { + readonly [key: string]: Record; + }[] | null; }; }; }; @@ -8199,6 +8207,11 @@ export interface operations { readonly widgets?: readonly { readonly [key: string]: Record; }[] | null; + /** + * @description New technical name, unique within the application + * @default null + */ + readonly technicalName?: string | null; }; }; }; @@ -8217,7 +8230,7 @@ export interface operations { }; }; }; - /** @description Invalid page field or widget */ + /** @description Invalid page field, technical name or widget */ readonly 400: { headers: { readonly [name: string]: unknown; @@ -8324,6 +8337,11 @@ export interface operations { * @default null */ readonly description?: string | null; + /** + * @description Name to address the page by, unique within the application; left out makes one from the title + * @default null + */ + readonly technicalName?: string | null; }; }; }; @@ -8342,7 +8360,7 @@ export interface operations { }; }; }; - /** @description Invalid title, emoji or description */ + /** @description Invalid title, emoji, description or technical name */ readonly 400: { headers: { readonly [name: string]: unknown; diff --git a/tests/unit/Controller/ContextPageControllerTest.php b/tests/unit/Controller/ContextPageControllerTest.php index 0bf720c4df..79b9e58653 100644 --- a/tests/unit/Controller/ContextPageControllerTest.php +++ b/tests/unit/Controller/ContextPageControllerTest.php @@ -59,16 +59,16 @@ public function testShowAnUnknownPageIsNotFound(): void { } public function testCreatePassesThePageFields(): void { - $this->pageService->expects($this->once())->method('create')->with(7, 'Overview', 'πŸ“Š', 'At a glance', 'alice')->willReturn([]); + $this->pageService->expects($this->once())->method('create')->with(7, 'Overview', 'πŸ“Š', 'At a glance', 'overview', 'alice')->willReturn([]); - $this->assertSame(Http::STATUS_OK, $this->controller->create(7, 'Overview', 'πŸ“Š', 'At a glance')->getStatus()); + $this->assertSame(Http::STATUS_OK, $this->controller->create(7, 'Overview', 'πŸ“Š', 'At a glance', 'overview')->getStatus()); } public function testUpdatePassesFieldsAndWidgets(): void { $widgets = [['type' => 'text', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]]; - $this->pageService->expects($this->once())->method('update')->with(7, 2, null, '', null, $widgets, 'alice')->willReturn([]); + $this->pageService->expects($this->once())->method('update')->with(7, 2, null, '', null, $widgets, 'alice', 'board')->willReturn([]); - $this->assertSame(Http::STATUS_OK, $this->controller->update(7, 2, null, '', null, $widgets)->getStatus()); + $this->assertSame(Http::STATUS_OK, $this->controller->update(7, 2, null, '', null, $widgets, 'board')->getStatus()); } public static function errors(): array { diff --git a/tests/unit/Db/ContextMapperTest.php b/tests/unit/Db/ContextMapperTest.php index 013a3ead18..50501b9685 100644 --- a/tests/unit/Db/ContextMapperTest.php +++ b/tests/unit/Db/ContextMapperTest.php @@ -61,10 +61,10 @@ 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); + $startPage = $this->addPage($context, Page::TYPE_STARTPAGE, null, null, null, 'dashboard'); $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'); + $gridPage = $this->addPage($context, Page::TYPE_GRID, 'Overview', 'πŸ“Š', 'Open work at a glance', 'overview'); $pages = $this->mapper->findById($context->getId(), self::OWNER)->getPages(); @@ -76,6 +76,7 @@ public function testGridPageHasItsFieldsAndNoStartPageContent(): void { 'title' => 'Overview', 'emoji' => 'πŸ“Š', 'description' => 'Open work at a glance', + 'technical_name' => 'overview', ], $pages[$gridPage->getId()]); $this->assertNull($pages[$startPage->getId()]['title']); $this->assertSame([$firstContent->getId(), $secondContent->getId()], array_keys($pages[$startPage->getId()]['content'])); @@ -102,13 +103,14 @@ private function addNode(Context $context, int $nodeId): ContextNodeRelation { return $this->nodeRelMapper->insert($relation); } - private function addPage(Context $context, string $type, ?string $title, ?string $emoji, ?string $description): Page { + private function addPage(Context $context, string $type, ?string $title, ?string $emoji, ?string $description, string $technicalName): Page { $page = new Page(); $page->setContextId($context->getId()); $page->setPageType($type); $page->setTitle($title); $page->setEmoji($emoji); $page->setDescription($description); + $page->setTechnicalName($technicalName); return $this->pageMapper->insert($page); } diff --git a/tests/unit/Helper/TechnicalNameHelperTest.php b/tests/unit/Helper/TechnicalNameHelperTest.php new file mode 100644 index 0000000000..f77b383e8b --- /dev/null +++ b/tests/unit/Helper/TechnicalNameHelperTest.php @@ -0,0 +1,87 @@ + ['Sales', 'sales'], + 'spaces and punctuation' => ['Open cases (2026)!', 'open_cases_2026'], + 'trimmed' => [' --Team overview-- ', 'team_overview'], + 'non-ascii' => ['CafΓ© ΓΌber', 'caf_ber'], + 'leading digit' => ['2026 plan', 'page_2026_plan'], + 'only punctuation' => ['!!!', 'page'], + 'empty' => ['', 'page'], + 'already valid' => ['open_cases_2', 'open_cases_2'], + ]; + } + + /** + * @dataProvider textProvider + */ + public function testFromText(string $text, string $expected): void { + $this->assertSame($expected, TechnicalNameHelper::fromText($text)); + } + + public function testFromTextUsesThePrefix(): void { + $this->assertSame('item', TechnicalNameHelper::fromText('', 'item')); + $this->assertSame('item_1st', TechnicalNameHelper::fromText('1st', 'item')); + } + + public function testFromTextIsCutToTheMaximumLength(): void { + $name = TechnicalNameHelper::fromText(str_repeat('a', 199) . ' b c'); + + $this->assertSame(str_repeat('a', 199), $name); + $this->assertTrue(TechnicalNameHelper::isValid($name)); + } + + public function testUniqueAddsTheLowestFreeSuffix(): void { + $this->assertSame('sales', TechnicalNameHelper::unique('sales', [])); + $this->assertSame('sales_2', TechnicalNameHelper::unique('sales', ['sales' => true])); + $this->assertSame('sales_3', TechnicalNameHelper::unique('sales', ['sales' => true, 'sales_2' => true])); + } + + public function testUniqueStaysWithinTheMaximumLength(): void { + $base = str_repeat('a', 200); + + $name = TechnicalNameHelper::unique($base, [$base => true]); + + $this->assertSame(str_repeat('a', 198) . '_2', $name); + } + + public function testAssignKeepsFreeNamesAndMakesTheRestUnique(): void { + $names = TechnicalNameHelper::assign([ + 'a' => ['name' => null, 'fallback' => 'dashboard'], + 'b' => ['name' => 'dashboard', 'fallback' => 'overview'], + 'c' => ['name' => 'sales', 'fallback' => 'sales'], + 'd' => ['name' => 'sales', 'fallback' => 'sales'], + 'e' => ['name' => 'Not Valid', 'fallback' => 'x'], + 'f' => ['name' => '', 'fallback' => 'archive'], + ], ['archive' => true]); + + $this->assertSame([ + 'a' => 'dashboard_2', + 'b' => 'dashboard', + 'c' => 'sales', + 'd' => 'sales_2', + 'e' => 'not_valid', + 'f' => 'archive_2', + ], $names); + } + + public function testPageFallback(): void { + $this->assertSame('dashboard', TechnicalNameHelper::pageFallback(true, 'Anything')); + $this->assertSame('team_overview', TechnicalNameHelper::pageFallback(false, 'Team overview')); + $this->assertSame('page', TechnicalNameHelper::pageFallback(false, null)); + } +} diff --git a/tests/unit/Migration/Version2400Date20261016000002Test.php b/tests/unit/Migration/Version2400Date20261016000002Test.php new file mode 100644 index 0000000000..faaaa74abb --- /dev/null +++ b/tests/unit/Migration/Version2400Date20261016000002Test.php @@ -0,0 +1,61 @@ + 1, 'context_id' => 10, 'page_type' => 'startpage', 'title' => null, 'technical_name' => null], + ['id' => 2, 'context_id' => 10, 'page_type' => 'grid', 'title' => 'Team overview', 'technical_name' => null], + ['id' => 3, 'context_id' => 10, 'page_type' => 'grid', 'title' => 'Team: overview', 'technical_name' => null], + ['id' => 4, 'context_id' => 10, 'page_type' => 'grid', 'title' => null, 'technical_name' => null], + ['id' => 5, 'context_id' => 10, 'page_type' => 'grid', 'title' => '!!', 'technical_name' => ''], + ['id' => 6, 'context_id' => 11, 'page_type' => 'startpage', 'title' => null, 'technical_name' => null], + ['id' => 7, 'context_id' => 11, 'page_type' => 'grid', 'title' => 'Team overview', 'technical_name' => null], + ]); + + $this->assertSame([ + 1 => 'dashboard', + 2 => 'team_overview', + 3 => 'team_overview_2', + 4 => 'page_4', + 5 => 'page_5', + 6 => 'dashboard', + 7 => 'team_overview', + ], $names); + } + + public function testBackfillKeepsFreeNamesAndRenamesDuplicates(): void { + $names = Version2400Date20261016000002::backfillNames([ + ['id' => 1, 'context_id' => 10, 'page_type' => 'startpage', 'title' => null, 'technical_name' => null], + ['id' => 2, 'context_id' => 10, 'page_type' => 'grid', 'title' => 'Board', 'technical_name' => 'dashboard'], + ['id' => 3, 'context_id' => 10, 'page_type' => 'grid', 'title' => 'Sales', 'technical_name' => 'sales'], + ['id' => 4, 'context_id' => 10, 'page_type' => 'grid', 'title' => 'Sales', 'technical_name' => 'sales'], + ['id' => 5, 'context_id' => 10, 'page_type' => 'grid', 'title' => 'Other', 'technical_name' => 'Bad Name'], + ]); + + $this->assertSame([ + 1 => 'dashboard_2', + 4 => 'sales_2', + 5 => 'bad_name', + ], $names); + } + + public function testBackfillLeavesNamedPagesAlone(): void { + $this->assertSame([], Version2400Date20261016000002::backfillNames([ + ['id' => 1, 'context_id' => 10, 'page_type' => 'startpage', 'title' => null, 'technical_name' => 'dashboard'], + ['id' => 2, 'context_id' => 10, 'page_type' => 'grid', 'title' => 'Sales', 'technical_name' => 'sales'], + ])); + $this->assertSame([], Version2400Date20261016000002::backfillNames([])); + } +} diff --git a/tests/unit/Service/ContextPagesRoundTripTest.php b/tests/unit/Service/ContextPagesRoundTripTest.php index a763019790..6a60256c65 100644 --- a/tests/unit/Service/ContextPagesRoundTripTest.php +++ b/tests/unit/Service/ContextPagesRoundTripTest.php @@ -50,6 +50,7 @@ class ContextPagesRoundTripTest extends DatabaseTestCase { private ContextNodeRelationMapper $nodeRelMapper; private PageMapper $pageMapper; private WidgetService $widgetService; + private TableMapper $tableMapper; /** @var int[] */ private array $contextIds = []; @@ -81,7 +82,7 @@ protected function setUp(): void { false, $this->createMock(INavigationManager::class), $this->createMock(IURLGenerator::class), - $this->createMock(TableMapper::class), + $this->tableMapper = $this->createMock(TableMapper::class), $this->createMock(ViewMapper::class), $this->widgetService, new GridWidgetTypes($l10n), @@ -119,7 +120,7 @@ public function testExportedWidgetsAreImportedWithTheirNode(): void { $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']]); + $this->assertSame(['Overview', 'πŸ“Š', 'At a glance', 'overview'], [$gridPages[0]['title'], $gridPages[0]['emoji'], $gridPages[0]['description'], $gridPages[0]['technical_name']]); $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 { @@ -148,6 +149,57 @@ public function testRemovingTheNodeKeepsTheWidgetAndDeletingTheContextRemovesIt( $this->assertSame([], $this->pageMapper->getPageIdsForContext($context->getId())); } + public function testImportedPagesAreNamedUniquelyWithinTheApplication(): void { + $table = new Table(); + $table->id = 4343; + $table->setOwnership(self::OWNER); + + $imported = $this->service->importContext($table, [ + 'name' => 'Named', + 'nodes' => [['id' => 1, 'node_id' => 4343, 'node_type' => Application::NODE_TYPE_TABLE, 'permissions' => 1]], + 'pages' => [ + ['id' => 1, 'page_type' => Page::TYPE_STARTPAGE, 'content' => []], + ['id' => 2, 'page_type' => Page::TYPE_GRID, 'title' => 'Sales board', 'content' => []], + ['id' => 3, 'page_type' => Page::TYPE_GRID, 'title' => 'Sales board', 'technical_name' => 'sales_board', 'content' => []], + ], + ], 4343); + $this->contextIds[] = $imported->getId(); + + $pages = array_values($this->contextMapper->findById($imported->getId(), self::OWNER)->getPages()); + usort($pages, static fn (array $a, array $b): int => $a['id'] <=> $b['id']); + $this->assertSame(['dashboard', 'sales_board_2', 'sales_board'], array_column($pages, 'technical_name')); + } + + public function testASchemeUpdatesThePageWithTheSameTechnicalNameAndAddsTheOthers(): void { + [$context, $relation, $gridPageId] = $this->createContextWithGridPage(); + $this->tableMapper->method('findByUuid')->with('table-uuid')->willReturnCallback(static function (): Table { + $table = new Table(); + $table->id = 4141; + return $table; + }); + + $this->service->importSchemePages($context->getId(), [ + ['id' => 900, 'node_type' => Application::NODE_TYPE_TABLE, 'node_uuid' => 'table-uuid', 'permissions' => 1], + ], [ + ['page_type' => Page::TYPE_GRID, 'technical_name' => 'overview', 'title' => 'Renamed', 'emoji' => null, 'description' => null, 'widgets' => [ + ['type' => 'data', 'title' => 'Tasks', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 12, 'gridHeight' => 5, 'settings' => [], 'source' => ['nodeRelId' => 900]], + ]], + ['page_type' => Page::TYPE_GRID, 'technical_name' => 'planning', 'title' => 'Planning', 'widgets' => [ + ['type' => 'text', 'title' => null, 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2, 'settings' => ['text' => 'Next'], 'source' => null], + ]], + ]); + + $pages = $this->service->getPagesWithWidgets($this->contextMapper->findById($context->getId(), self::OWNER)); + $this->assertCount(3, $pages); + $this->assertSame('Renamed', $pages[$gridPageId]['title']); + $this->assertCount(1, $pages[$gridPageId]['widgets']); + $this->assertSame(['nodeRelId' => $relation->getId()], $pages[$gridPageId]['widgets'][0]['source']); + $planning = array_values(array_filter($pages, static fn (array $page): bool => $page['technical_name'] === 'planning')); + $this->assertCount(1, $planning); + $this->assertSame(Page::TYPE_GRID, $planning[0]['page_type']); + $this->assertSame(['text' => 'Next'], $planning[0]['widgets'][0]['settings']); + } + /** * @return array{0: Context, 1: ContextNodeRelation, 2: int} */ @@ -163,6 +215,7 @@ private function createContextWithGridPage(): array { $page->setTitle('Overview'); $page->setEmoji('πŸ“Š'); $page->setDescription('At a glance'); + $page->setTechnicalName('overview'); $page = $this->pageMapper->insert($page); $this->widgetService->insert($page->getId(), 'header', 'Welcome', ['gridX' => 0, 'gridY' => 0, 'gridWidth' => 12, 'gridHeight' => 2], ['heading' => 'Projects', 'subheading' => '', 'textAlign' => 'center', 'backgroundColor' => '#123456', 'textColor' => ''], null); diff --git a/tests/unit/Service/PageServiceTest.php b/tests/unit/Service/PageServiceTest.php index daf1894fcb..4533f5d7d6 100644 --- a/tests/unit/Service/PageServiceTest.php +++ b/tests/unit/Service/PageServiceTest.php @@ -49,8 +49,8 @@ protected function setUp(): void { $this->context->id = 7; $this->context->setNodes([self::NODE_REL => ['id' => self::NODE_REL, 'node_id' => 3, 'node_type' => Application::NODE_TYPE_TABLE, 'permissions' => 1]]); $this->context->setPages([ - self::START_PAGE => ['id' => self::START_PAGE, 'page_type' => Page::TYPE_STARTPAGE, 'title' => null, 'emoji' => null, 'description' => null, 'content' => []], - self::GRID_PAGE => ['id' => self::GRID_PAGE, 'page_type' => Page::TYPE_GRID, 'title' => 'Overview', 'emoji' => 'πŸ“Š', 'description' => null, 'content' => []], + self::START_PAGE => ['id' => self::START_PAGE, 'page_type' => Page::TYPE_STARTPAGE, 'title' => null, 'emoji' => null, 'description' => null, 'technical_name' => 'dashboard', 'content' => []], + self::GRID_PAGE => ['id' => self::GRID_PAGE, 'page_type' => Page::TYPE_GRID, 'title' => 'Overview', 'emoji' => 'πŸ“Š', 'description' => null, 'technical_name' => 'overview', 'content' => []], ]); $contextMapper = $this->createMock(ContextMapper::class); $contextMapper->method('findById')->willReturnCallback(function (int $contextId): Context { @@ -91,6 +91,7 @@ public function testFindReturnsThePageWithItsWidgets(): void { 'title' => 'Overview', 'emoji' => 'πŸ“Š', 'description' => null, + 'technicalName' => 'overview', 'widgets' => [$widget], ], $this->service->find(7, self::GRID_PAGE, 'alice')); } @@ -109,9 +110,10 @@ public function testCreateInsertsAGridPage(): void { return $page; }); - $this->service->create(7, ' Overview ', ' πŸ“Š ', '', 'alice'); + $this->service->create(7, ' Overview ', ' πŸ“Š ', '', null, 'alice'); $this->assertSame([7, Page::TYPE_GRID, 'Overview', 'πŸ“Š', null], [$inserted->getContextId(), $inserted->getPageType(), $inserted->getTitle(), $inserted->getEmoji(), $inserted->getDescription()]); + $this->assertSame('overview_2', $inserted->getTechnicalName()); } public function testCreateNeedsManagePermission(): void { @@ -119,7 +121,7 @@ public function testCreateNeedsManagePermission(): void { $this->pageMapper->expects($this->never())->method('insert'); $this->expectException(PermissionError::class); - $this->service->create(7, 'Overview', null, null, 'bob'); + $this->service->create(7, 'Overview', null, null, null, 'bob'); } public function testCreateNeedsATitle(): void { @@ -127,7 +129,66 @@ public function testCreateNeedsATitle(): void { $this->pageMapper->expects($this->never())->method('insert'); $this->expectException(BadRequestError::class); - $this->service->create(7, ' ', null, null, 'alice'); + $this->service->create(7, ' ', null, null, null, 'alice'); + } + + public static function invalidTechnicalNames(): array { + return [ + 'upper case' => ['Overview', 'starts with a lowercase letter'], + 'starts with a digit' => ['1st', 'starts with a lowercase letter'], + 'taken by another page' => ['dashboard', 'already uses the technical name dashboard'], + ]; + } + + /** + * @dataProvider invalidTechnicalNames + */ + public function testCreateRejectsAnInvalidOrTakenTechnicalName(string $technicalName, string $message): void { + $this->permissionsService->method('canManageContextById')->willReturn(true); + $this->pageMapper->expects($this->never())->method('insert'); + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage($message); + $this->service->create(7, 'Board', null, null, $technicalName, 'alice'); + } + + public function testCreateKeepsAGivenTechnicalName(): void { + $this->permissionsService->method('canManageContextById')->willReturn(true); + $this->pageMapper->expects($this->once())->method('insert')->willReturnCallback(function (Page $page): Page { + $this->assertSame('sales_board', $page->getTechnicalName()); + $page->id = self::GRID_PAGE; + return $page; + }); + + $this->service->create(7, 'Board', null, null, 'sales_board', 'alice'); + } + + public function testARaceOnATechnicalNameIsABadRequest(): void { + $this->permissionsService->method('canManageContextById')->willReturn(true); + $violation = new class('duplicate') extends \OCP\DB\Exception { + public function getReason(): ?int { + return self::REASON_UNIQUE_CONSTRAINT_VIOLATION; + } + }; + $this->pageMapper->method('insert')->willThrowException($violation); + + $this->expectException(BadRequestError::class); + $this->service->create(7, 'Board', null, null, 'board', 'alice'); + } + + public function testUpdateMayKeepItsOwnTechnicalName(): void { + $this->permissionsService->method('canManageContextById')->willReturn(true); + $stored = new Page(); + $stored->id = self::GRID_PAGE; + $stored->setTechnicalName('overview'); + $stored->resetUpdatedFields(); + $this->pageMapper->method('findById')->willReturn($stored); + $this->pageMapper->expects($this->never())->method('update'); + + $this->service->update(7, self::GRID_PAGE, null, null, null, null, 'alice', 'overview'); + + $this->expectException(BadRequestError::class); + $this->service->update(7, self::GRID_PAGE, null, null, null, null, 'alice', 'dashboard'); } public function testUpdateSyncsWidgetsById(): void {