diff --git a/cypress/component/menuItems.cy.js b/cypress/component/menuItems.cy.js new file mode 100644 index 0000000000..ad68aa1952 --- /dev/null +++ b/cypress/component/menuItems.cy.js @@ -0,0 +1,118 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +import { + areMenuItemsValid, + canIndent, + canMove, + dragItemOver, + indentItem, + isValidMenuLink, + menuItemErrors, + moveItem, + outdentItem, + parentOfPosition, + removeItem, + toMenuItemPayload, +} from '../../src/shared/utils/menuItems.js' + +/** + * An item of the editor's list. + * + * @param {string} key the item's key, also used as its label + * @param {string|null} parentKey the key of its parent, or null at the top level + * @return {object} the item + */ +function item(key, parentKey = null) { + return { key, label: key, icon: null, targetType: 'url', targetId: null, url: 'https://example.org/' + key, technicalName: null, parentKey } +} + +/** + * The list as "key" for a top-level item and "parent>key" for a child, to read the shape at a glance. + * + * @param {Array} items the list + * @return {Array} the shape of the list + */ +function shape(items) { + return items.map(entry => entry.parentKey === null ? entry.key : entry.parentKey + '>' + entry.key) +} + +// A with its child a1, then B and C at the top level +const menu = () => [item('A'), item('a1', 'A'), item('B'), item('C')] + +describe('menu items', () => { + it('sends a child with the position of its parent', () => { + const items = menu() + + expect(toMenuItemPayload(items[1], items).parentIndex).to.equal(0) + expect(toMenuItemPayload(items[2], items).parentIndex).to.equal(null) + expect(toMenuItemPayload(items[2], items).targetId).to.equal(null) + }) + + it('accepts only the links the server accepts', () => { + expect(isValidMenuLink('https://example.org/docs')).to.equal(true) + expect(isValidMenuLink('http://example.org')).to.equal(true) + expect(isValidMenuLink('www.example.org')).to.equal(false) + expect(isValidMenuLink('https://')).to.equal(false) + expect(isValidMenuLink('javascript://example.com/%0Aalert(1)')).to.equal(false) + expect(isValidMenuLink('https://example.org/' + 'x'.repeat(2000))).to.equal(false) + }) + + it('marks an item without a label, without a target or with a bad link', () => { + expect(menuItemErrors({ ...item('A'), label: ' ' }).label).to.equal(true) + expect(menuItemErrors({ ...item('A'), targetType: 'view', targetId: null }).target).to.equal(true) + expect(menuItemErrors({ ...item('A'), url: 'www.example.org' }).url).to.equal(true) + expect(menuItemErrors(item('A'))).to.deep.equal({ label: false, target: false, url: false }) + expect(areMenuItemsValid(menu())).to.equal(true) + expect(areMenuItemsValid([...menu(), { ...item('D'), url: '' }])).to.equal(false) + }) + + it('moves a top-level item together with its children', () => { + expect(shape(moveItem(menu(), 2, -1))).to.deep.equal(['B', 'A', 'A>a1', 'C']) + expect(shape(moveItem(menu(), 0, 1))).to.deep.equal(['B', 'A', 'A>a1', 'C']) + expect(canMove(menu(), 0, -1)).to.equal(false) + expect(canMove(menu(), 1, 1)).to.equal(false, 'a1 is the only child of A') + }) + + it('nests one level deep and only under the top-level item above', () => { + expect(canIndent(menu(), 0)).to.equal(false, 'nothing above A') + expect(canIndent(menu(), 2)).to.equal(true) + expect(shape(indentItem(menu(), 2))).to.deep.equal(['A', 'A>a1', 'A>B', 'C']) + expect(canIndent([item('A'), item('B'), item('b1', 'B')], 1)).to.equal(false, 'B has a child itself') + }) + + it('puts an outdented child after the last child of its parent', () => { + const items = [item('A'), item('a1', 'A'), item('a2', 'A'), item('B')] + + expect(shape(outdentItem(items, 1))).to.deep.equal(['A', 'A>a2', 'a1', 'B']) + }) + + it('moves the children of a removed item to the top level', () => { + expect(shape(removeItem(menu(), 0))).to.deep.equal(['a1', 'B', 'C']) + expect(shape(removeItem(menu(), 1))).to.deep.equal(['A', 'B', 'C']) + }) + + it('knows which parent a position belongs to', () => { + const rest = [item('A'), item('a1', 'A'), item('B')] + + expect(parentOfPosition(rest, 0)).to.equal(null) + expect(parentOfPosition(rest, 1)?.key).to.equal('A') + expect(parentOfPosition(rest, 2)?.key).to.equal('A') + expect(parentOfPosition(rest, 3)).to.equal(null) + }) + + it('drags a child into another parent and a top-level block past children', () => { + const items = [item('A'), item('a1', 'A'), item('B'), item('b1', 'B')] + + const childMoved = dragItemOver(items, 1, 3) + expect(shape(childMoved.items)).to.deep.equal(['A', 'B', 'B>b1', 'B>a1']) + expect(childMoved.draggedIndex).to.equal(3) + + const blockMoved = dragItemOver(items, 2, 1) + expect(shape(blockMoved.items)).to.deep.equal(['A', 'A>a1', 'B', 'B>b1'], 'B cannot land between A and its child') + + expect(dragItemOver(items, 0, 1)).to.equal(null, 'a block cannot be dropped on its own child') + expect(dragItemOver(items, 2, 2)).to.equal(null) + }) +}) diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 032f30a9c8..f0459dafaf 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -567,7 +567,10 @@ private function applyUpdate(int $contextId, string $userId, ?string $name, ?str public function delete(int $contextId, string $userId): Context { $context = $this->contextMapper->findById($contextId, $userId); - $this->atomic(function () use ($context): void { + // The application goes first, so the transaction holds the write lock before it reads: on + // SQLite a transaction that reads first fails at once when another request writes meanwhile + $this->atomicRetry(function () use ($context): void { + $this->contextMapper->delete($context); $this->shareService->deleteAllForContext($context); $this->menuItemMapper->deleteAllByContextId($context->getId()); $this->contextNodeRelMapper->deleteAllByContextId($context->getId()); @@ -576,7 +579,6 @@ public function delete(int $contextId, string $userId): Context { $this->pageContentMapper->deleteByPageId($pageId); $this->pageMapper->deleteByPageId($pageId); } - $this->contextMapper->delete($context); }, $this->dbc); return $context; } diff --git a/src/modules/modals/CreateContext.vue b/src/modules/modals/CreateContext.vue index 9b56275a70..d5ab502808 100644 --- a/src/modules/modals/CreateContext.vue +++ b/src/modules/modals/CreateContext.vue @@ -42,6 +42,12 @@ +
+
+ {{ t('tables', 'Menu') }} +
+ +
@@ -54,7 +60,10 @@
- + {{ t('tables', 'Create application') }}
@@ -68,6 +77,8 @@ import { NcDialog, NcButton, NcIconSvgWrapper, NcActionCheckbox } from '@nextclo import { showError } from '@nextcloud/dialogs' import '@nextcloud/dialogs/style.css' import NcContextResource from '../../shared/components/ncContextResource/NcContextResource.vue' +import MenuItemsEditor from '../../shared/components/ncContextResource/MenuItemsEditor.vue' +import { areMenuItemsValid, toMenuItemPayload } from '../../shared/utils/menuItems.js' import NcIconPicker from '../../shared/components/ncIconPicker/NcIconPicker.vue' import svgHelper from '../../shared/components/ncIconPicker/mixins/svgHelper.js' import permissionBitmask from '../../shared/components/ncContextResource/mixins/permissionBitmask.js' @@ -84,6 +95,7 @@ export default { NcButton, NcIconSvgWrapper, NcContextResource, + MenuItemsEditor, NcActionCheckbox, }, mixins: [svgHelper, permissionBitmask], @@ -106,11 +118,22 @@ export default { customTitleChosen: false, errorTitle: false, description: '', + menuItems: [], resources: [], receivers: [], showInNavigationDefault: false, } }, + computed: { + /** + * Whether every menu item can be saved, so a half-filled item does not fail the whole application. + * + * @return {boolean} true when no menu item has an error + */ + menuItemsValid() { + return areMenuItemsValid(this.menuItems) + }, + }, watch: { title() { if (this.title.length >= 200) { @@ -141,6 +164,11 @@ export default { this.$emit('close') }, async submit() { + // Show every menu error now, and never send a menu the server would refuse + this.$refs.menuItemsEditor?.touchAll() + if (!this.menuItemsValid) { + return + } if (this.title === '') { showError(t('tables', 'Cannot create new application. Title is missing.')) this.errorTitle = true @@ -169,6 +197,7 @@ export default { iconName: this.icon.name, description: this.description, nodes: dataResources, + menuItems: this.menuItems.map(item => toMenuItemPayload(item, this.menuItems)), } // adding share to oneself to have navigation display control this.receivers.push( @@ -192,6 +221,7 @@ export default { }, reset() { this.title = '' + this.menuItems = [] this.errorTitle = false this.setIcon(this.randomIcon()) this.customTitleChosen = false diff --git a/src/shared/components/ApplicationIcon.vue b/src/shared/components/ApplicationIcon.vue new file mode 100644 index 0000000000..e01608b791 --- /dev/null +++ b/src/shared/components/ApplicationIcon.vue @@ -0,0 +1,55 @@ + + + + diff --git a/src/shared/components/ncContextResource/MenuItemsEditor.vue b/src/shared/components/ncContextResource/MenuItemsEditor.vue new file mode 100644 index 0000000000..25ef6204f9 --- /dev/null +++ b/src/shared/components/ncContextResource/MenuItemsEditor.vue @@ -0,0 +1,454 @@ + + + + + + diff --git a/src/shared/utils/menuItems.js b/src/shared/utils/menuItems.js new file mode 100644 index 0000000000..58d0b88dae --- /dev/null +++ b/src/shared/utils/menuItems.js @@ -0,0 +1,300 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +/* + * The menu of an application as the editor holds it: a flat, ordered list where a child carries + * the key of its parent. A parent always precedes its children, there is one level of nesting, + * and a parent that moves takes its children along. Every function returns a new list. + */ + +// The same limits as MenuItemInput on the server +export const MAX_LABEL_LENGTH = 200 +export const MAX_URL_LENGTH = 2000 + +/** + * The payload the API expects for one menu item. + * + * @param {object} item an item from the editor + * @param {Array} items the whole editable list, to turn the parent key into the parent's position + * @return {object} the item as the API takes it + */ +export function toMenuItemPayload(item, items) { + const parentIndex = item.parentKey ? items.findIndex(candidate => candidate.key === item.parentKey) : -1 + return { + label: item.label, + icon: item.icon || null, + targetType: item.targetType, + targetId: item.targetType === 'url' ? null : item.targetId, + url: item.targetType === 'url' ? item.url : null, + technicalName: item.technicalName || null, + parentIndex: parentIndex >= 0 ? parentIndex : null, + } +} + +/** + * A new, empty top-level item. + * + * @param {Array} items the current list, to give the item its own key + * @return {object} the new item + */ +export function newMenuItem(items) { + return { + // Two items added within one millisecond still get their own key + key: 'new-' + Date.now() + '-' + items.length, + label: '', + icon: null, + targetType: 'url', + targetId: null, + url: '', + technicalName: null, + parentKey: null, + } +} + +/** + * Whether a link is an http or https address with a host, as the server requires. + * + * @param {string|null} url the link + * @return {boolean} true when the server accepts it + */ +export function isValidMenuLink(url) { + if (typeof url !== 'string' || url.length > MAX_URL_LENGTH) { + return false + } + try { + const parsed = new URL(url.trim()) + return ['http:', 'https:'].includes(parsed.protocol) && parsed.hostname !== '' + } catch { + return false + } +} + +/** + * What is missing or wrong in an item, by field, with the same rules as the server. + * + * @param {object} item an item from the editor + * @return {{label: boolean, target: boolean, url: boolean}} true for every field that is invalid + */ +export function menuItemErrors(item) { + const label = (item.label ?? '').trim() + return { + label: label === '' || label.length > MAX_LABEL_LENGTH, + target: item.targetType !== 'url' && !(item.targetId > 0), + url: item.targetType === 'url' && !isValidMenuLink(item.url), + } +} + +/** + * Whether every item of the menu can be saved. + * + * @param {Array} items the editable list + * @return {boolean} true when no item has an error + */ +export function areMenuItemsValid(items) { + return items.every(item => !Object.values(menuItemErrors(item)).includes(true)) +} + +/** + * The nearest top-level item above a position, which is the only possible parent. + * + * @param {Array} items the editable list + * @param {number} index position of the item + * @return {object|null} the possible parent, or null at the top of the list + */ +export function parentAbove(items, index) { + for (let i = index - 1; i >= 0; i--) { + if (items[i].parentKey === null) { + return items[i] + } + } + return null +} + +/** + * Whether an item has children. + * + * @param {Array} items the editable list + * @param {object} item an item of the list + * @return {boolean} true when another item sits under it + */ +export function hasChildren(items, item) { + return items.some(candidate => candidate.parentKey === item.key) +} + +/** + * Whether an item can move under the top-level item above it: only one level of nesting. + * + * @param {Array} items the editable list + * @param {number} index position of the item + * @return {boolean} true when it can become a child + */ +export function canIndent(items, index) { + return parentAbove(items, index) !== null && !hasChildren(items, items[index]) +} + +/** + * Moves an item under the top-level item above it. + * + * @param {Array} items the editable list + * @param {number} index position of the item + * @return {Array} the new list, or the same list when the item cannot move + */ +export function indentItem(items, index) { + if (!canIndent(items, index)) { + return items + } + const parent = parentAbove(items, index) + return items.map((item, i) => i === index ? { ...item, parentKey: parent.key } : item) +} + +/** + * Moves a child to the top level, right after its parent's last child. + * + * @param {Array} items the editable list + * @param {number} index position of the child + * @return {Array} the new list + */ +export function outdentItem(items, index) { + const result = [...items] + const [moved] = result.splice(index, 1) + let insertAt = result.findIndex(item => item.key === moved.parentKey) + 1 + while (insertAt < result.length && result[insertAt].parentKey === moved.parentKey) { + insertAt++ + } + result.splice(insertAt, 0, { ...moved, parentKey: null }) + return result +} + +/** + * Removes an item; its children move to the top level instead of disappearing. + * + * @param {Array} items the editable list + * @param {number} index position of the item + * @return {Array} the new list + */ +export function removeItem(items, index) { + const removed = items[index] + return items + .filter((item, i) => i !== index) + .map(item => item.parentKey === removed.key ? { ...item, parentKey: null } : item) +} + +/** + * An item with its children, as one block that moves together. + * + * @param {Array} items the editable list + * @param {number} index position of the item + * @return {Array} the item followed by its children + */ +export function blockOf(items, index) { + const item = items[index] + if (item.parentKey !== null) { + return [item] + } + return [item, ...items.filter(candidate => candidate.parentKey === item.key)] +} + +/** + * The items an item shares its level with: the top-level items, or the children of its parent. + * + * @param {Array} items the editable list + * @param {object} item an item of the list + * @return {Array} the items on the same level, in order + */ +export function siblingsOf(items, item) { + return items.filter(candidate => candidate.parentKey === item.parentKey) +} + +/** + * Whether an item has a neighbour on its level in the given direction. + * + * @param {Array} items the editable list + * @param {number} index position of the item + * @param {number} step -1 to move up, 1 to move down + * @return {boolean} true when it can move that way + */ +export function canMove(items, index, step) { + const siblings = siblingsOf(items, items[index]) + return siblings[siblings.indexOf(items[index]) + step] !== undefined +} + +/** + * Swaps an item with its neighbour on the same level; a top-level item moves with its children. + * + * @param {Array} items the editable list + * @param {number} index position of the item + * @param {number} step -1 to move up, 1 to move down + * @return {Array} the new list, or the same list when there is no neighbour + */ +export function moveItem(items, index, step) { + const item = items[index] + const siblings = siblingsOf(items, item) + const neighbour = siblings[siblings.indexOf(item) + step] + if (!neighbour) { + return items + } + const [first, second] = step < 0 ? [neighbour, item] : [item, neighbour] + const firstBlock = blockOf(items, items.indexOf(first)) + const secondBlock = blockOf(items, items.indexOf(second)) + const rest = items.filter(candidate => !firstBlock.includes(candidate) && !secondBlock.includes(candidate)) + rest.splice(items.indexOf(first), 0, ...secondBlock, ...firstBlock) + return rest +} + +/** + * The parent an item inserted at a position would belong to, or null at the top level. + * + * @param {Array} items the list without the item being moved + * @param {number} position where the item would be inserted + * @return {object|null} the parent at that position + */ +export function parentOfPosition(items, position) { + const previous = items[position - 1] + if (!previous) { + return null + } + if (previous.parentKey !== null) { + return items.find(item => item.key === previous.parentKey) ?? null + } + const next = items[position] + return next && next.parentKey === previous.key ? previous : null +} + +/** + * The list while an item is dragged over another one: a child joins the parent whose block it + * lands in, and a top-level block stays top-level, behind the children it would land between. + * + * @param {Array} items the editable list + * @param {number} draggedIndex position of the dragged item + * @param {number} overIndex position of the item it is dragged over + * @return {{items: Array, draggedIndex: number}|null} the new list and the dragged item's new position, or null when nothing changes + */ +export function dragItemOver(items, draggedIndex, overIndex) { + if (draggedIndex === overIndex) { + return null + } + const moving = blockOf(items, draggedIndex) + const over = items[overIndex] + if (moving.includes(over)) { + return null + } + const rest = items.filter(item => !moving.includes(item)) + let insertAt = rest.indexOf(over) + if (overIndex > draggedIndex) { + insertAt += 1 + } + const head = moving[0] + const landingParent = parentOfPosition(rest, insertAt) + const placed = head.parentKey !== null + ? [{ ...head, parentKey: landingParent?.key ?? null }] + : moving + if (head.parentKey === null && landingParent !== null) { + while (insertAt < rest.length && rest[insertAt].parentKey !== null) { + insertAt++ + } + } + rest.splice(insertAt, 0, ...placed) + return { items: rest, draggedIndex: rest.indexOf(placed[0]) } +} diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index cc66f14d64..57499c576f 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -373,6 +373,29 @@ public function testDeleteRemovesTheMenuOfTheApplication(): void { $this->service->delete(7, 'alice'); } + /** + * On SQLite a transaction that reads before it writes fails at once when another request + * writes meanwhile, so the delete takes the write lock first. + */ + public function testDeleteRemovesTheApplicationBeforeWhatBelongsToIt(): void { + $context = new Context(); + $context->id = 7; + $this->contextMapper->method('findById')->willReturn($context); + $this->pageMapper->method('getPageIdsForContext')->willReturn([]); + $calls = []; + $this->contextMapper->method('delete')->willReturnCallback(static function (Context $deleted) use (&$calls): Context { + $calls[] = 'application'; + return $deleted; + }); + $this->menuItemMapper->method('deleteAllByContextId')->willReturnCallback(static function () use (&$calls): void { + $calls[] = 'menu'; + }); + + $this->service->delete(7, 'alice'); + + $this->assertSame(['application', 'menu'], $calls); + } + public function testUpdateReordersStartPageContentsFromSubmittedNodes(): void { $context = new Context(); $context->id = 7;