diff --git a/apps/files/src/actions/deleteAction.spec.ts b/apps/files/src/actions/deleteAction.spec.ts index 27751cc8d0742..317e7848ad6d7 100644 --- a/apps/files/src/actions/deleteAction.spec.ts +++ b/apps/files/src/actions/deleteAction.spec.ts @@ -4,6 +4,7 @@ */ import type { IView } from '@nextcloud/files' +import type { TriggeredActionContext } from '../utils/actionUtils.ts' import axios from '@nextcloud/axios' import * as capabilities from '@nextcloud/capabilities' @@ -336,6 +337,110 @@ describe('Delete action execute tests', () => { expect(eventBus.emit).toBeCalledWith('files:node:deleted', file) }) + test('Delete action triggered by hotkey asks for confirmation', async () => { + // The confirmation dialog is disabled + expect(shouldAskForConfirmation()).toBe(false) + + vi.spyOn(axios, 'delete') + vi.spyOn(eventBus, 'emit') + + // Emulate the confirmation dialog to always confirm + const confirmMock = vi.fn().mockImplementation((a, b, c, resolve) => resolve(true)) + window.OC = { dialogs: { confirmDestructive: confirmMock } } + + const file = new File({ + id: 1, + source: 'https://cloud.domain.com/remote.php/dav/files/test/foobar.txt', + owner: 'test', + mime: 'text/plain', + permissions: Permission.READ | Permission.UPDATE | Permission.WRITE | Permission.DELETE, + root: '/files/test', + }) + + const exec = await action.exec({ + nodes: [file], + view, + folder: {} as Folder, + contents: [], + trigger: 'hotkey', + } as TriggeredActionContext) + + expect(confirmMock).toBeCalledTimes(1) + + expect(exec).toBe(true) + expect(axios.delete).toBeCalledTimes(1) + expect(axios.delete).toBeCalledWith('https://cloud.domain.com/remote.php/dav/files/test/foobar.txt') + + expect(eventBus.emit).toBeCalledTimes(1) + expect(eventBus.emit).toBeCalledWith('files:node:deleted', file) + }) + + test('Delete action triggered by hotkey is cancelled', async () => { + vi.spyOn(axios, 'delete') + vi.spyOn(eventBus, 'emit') + + // Emulate the confirmation dialog to always cancel + const confirmMock = vi.fn().mockImplementation((a, b, c, resolve) => resolve(false)) + window.OC = { dialogs: { confirmDestructive: confirmMock } } + + const file = new File({ + id: 1, + source: 'https://cloud.domain.com/remote.php/dav/files/test/foobar.txt', + owner: 'test', + mime: 'text/plain', + permissions: Permission.READ | Permission.UPDATE | Permission.WRITE | Permission.DELETE, + root: '/files/test', + }) + + const exec = await action.exec({ + nodes: [file], + view, + folder: {} as Folder, + contents: [], + trigger: 'hotkey', + } as TriggeredActionContext) + + expect(confirmMock).toBeCalledTimes(1) + + expect(exec).toBe(null) + expect(axios.delete).toBeCalledTimes(0) + expect(eventBus.emit).toBeCalledTimes(0) + }) + + test('Delete action triggered from the menu does not ask for confirmation', async () => { + // The confirmation dialog is disabled + expect(shouldAskForConfirmation()).toBe(false) + + vi.spyOn(axios, 'delete') + vi.spyOn(eventBus, 'emit') + + const confirmMock = vi.fn() + window.OC = { dialogs: { confirmDestructive: confirmMock } } + + const file = new File({ + id: 1, + source: 'https://cloud.domain.com/remote.php/dav/files/test/foobar.txt', + owner: 'test', + mime: 'text/plain', + permissions: Permission.READ | Permission.UPDATE | Permission.WRITE | Permission.DELETE, + root: '/files/test', + }) + + const exec = await action.exec({ + nodes: [file], + view, + folder: {} as Folder, + contents: [], + trigger: 'menu', + } as TriggeredActionContext) + + expect(confirmMock).toBeCalledTimes(0) + + expect(exec).toBe(true) + expect(axios.delete).toBeCalledTimes(1) + expect(axios.delete).toBeCalledWith('https://cloud.domain.com/remote.php/dav/files/test/foobar.txt') + }) + test('Delete action batch', async () => { vi.spyOn(axios, 'delete') vi.spyOn(eventBus, 'emit') diff --git a/apps/files/src/actions/deleteAction.ts b/apps/files/src/actions/deleteAction.ts index 582820199ac56..b5e06f85c7f15 100644 --- a/apps/files/src/actions/deleteAction.ts +++ b/apps/files/src/actions/deleteAction.ts @@ -4,6 +4,7 @@ */ import type { IFileAction } from '@nextcloud/files' +import type { TriggeredActionContext } from '../utils/actionUtils.ts' import CloseSvg from '@mdi/svg/svg/close.svg?raw' import NetworkOffSvg from '@mdi/svg/svg/network-off.svg?raw' @@ -51,17 +52,12 @@ export const action: IFileAction = { .every((permission) => (permission & Permission.DELETE) !== 0) }, - async exec({ nodes, view }) { + async exec({ nodes, view, trigger }: TriggeredActionContext) { try { let confirm = true - // Trick to detect if the action was called from a keyboard event - // we need to make sure the method calling have its named containing 'keydown' - // here we use `onKeydown` method from the FileEntryActions component - const callStack = new Error().stack || '' - const isCalledFromEventListener = callStack.toLocaleLowerCase().includes('keydown') - - if (shouldAskForConfirmation() || isCalledFromEventListener) { + // Deleting via the hotkey is easy to trigger by accident, so always confirm it + if (shouldAskForConfirmation() || trigger === 'hotkey') { confirm = await askConfirmation([nodes[0]], view) } diff --git a/apps/files/src/components/FileEntry/FileEntryActions.vue b/apps/files/src/components/FileEntry/FileEntryActions.vue index ea00068325225..7d27e86163af2 100644 --- a/apps/files/src/components/FileEntry/FileEntryActions.vue +++ b/apps/files/src/components/FileEntry/FileEntryActions.vue @@ -338,7 +338,7 @@ export default defineComponent({ this.activeStore.activeNode = this.source // Execute the action - await executeAction(action) + await executeAction(action, 'menu') }, onKeyDown(event: KeyboardEvent) { diff --git a/apps/files/src/composables/useHotKeys.spec.ts b/apps/files/src/composables/useHotKeys.spec.ts index a675bb2f5e35a..4b9cf837eef92 100644 --- a/apps/files/src/composables/useHotKeys.spec.ts +++ b/apps/files/src/composables/useHotKeys.spec.ts @@ -145,6 +145,17 @@ describe('HotKeysService testing', () => { expect(deleteAction.exec).toHaveBeenCalledOnce() }) + it('passes the hotkey trigger to the action', () => { + component.destroy() + registerFileAction(deleteAction) + component = mount(TestComponent) + + dispatchEvent({ key: 'Delete', code: 'Delete' }) + + expect(deleteAction.exec).toHaveBeenCalledOnce() + expect(deleteAction.exec).toHaveBeenCalledWith(expect.objectContaining({ trigger: 'hotkey' })) + }) + // actions implemented by the composable it('Pressing alt+up should go to parent directory', () => { diff --git a/apps/files/src/composables/useHotKeys.ts b/apps/files/src/composables/useHotKeys.ts index 9630ac5ae74e2..17f189bce58cc 100644 --- a/apps/files/src/composables/useHotKeys.ts +++ b/apps/files/src/composables/useHotKeys.ts @@ -33,7 +33,7 @@ export function useHotKeys(): void { : action.hotkey.key logger.debug(`Register hotkey for action "${action.id}"`) - useHotKey(key, () => executeAction(action), { + useHotKey(key, () => executeAction(action, 'hotkey'), { stop: true, prevent: true, alt: action.hotkey.alt, diff --git a/apps/files/src/utils/actionUtils.ts b/apps/files/src/utils/actionUtils.ts index 9fe659630a039..93b4169b95926 100644 --- a/apps/files/src/utils/actionUtils.ts +++ b/apps/files/src/utils/actionUtils.ts @@ -11,12 +11,26 @@ import Vue from 'vue' import { useActiveStore } from '../store/active.ts' import { logger } from '../utils/logger.ts' +/** + * How the execution of an action was triggered. + * `hotkey` means the action was triggered by a keyboard shortcut, + * `menu` means it was triggered from the actions menu or an inline action button. + */ +export type ActionTrigger = 'hotkey' | 'menu' + +/** + * Action context enriched with the trigger that started the execution. + * The trigger is undefined if the action was executed programmatically. + */ +export type TriggeredActionContext = ActionContextSingle & { trigger?: ActionTrigger } + /** * Execute an action on the current active node * * @param action The action to execute + * @param trigger How the execution was triggered */ -export async function executeAction(action: IFileAction) { +export async function executeAction(action: IFileAction, trigger?: ActionTrigger) { const activeStore = useActiveStore() const currentFolder = activeStore.activeFolder const currentNode = activeStore.activeNode @@ -39,7 +53,8 @@ export async function executeAction(action: IFileAction) { view: currentView, folder: currentFolder, contents, - } as ActionContextSingle + trigger, + } as TriggeredActionContext if (!action.enabled!(context)) { logger.debug('Action is not not available for the current context', { action, node: currentNode, view: currentView }) diff --git a/tests/playwright/e2e/files/hotkeys.spec.ts b/tests/playwright/e2e/files/hotkeys.spec.ts index 92e4c6f83e27c..7ab35ba94f49e 100644 --- a/tests/playwright/e2e/files/hotkeys.spec.ts +++ b/tests/playwright/e2e/files/hotkeys.spec.ts @@ -62,13 +62,31 @@ test.describe('Files hotkey handling', () => { await expect(filesListPage.getFavoriteIconForFile('abcd')).toHaveCount(0) }) - test('Pressing DELETE should delete the folder', async ({ page, filesListPage }) => { + test('Pressing DELETE should delete the folder after confirmation', async ({ page, filesListPage }) => { await filesListPage.getFilesList().press('ArrowDown') await expect(page).toHaveURL(/\/apps\/files\/files\/\d+/) await expect(filesListPage.getRows()).toHaveCount(2) await filesListPage.getFilesList().press('Delete') + await page.getByRole('dialog', { name: 'Confirm deletion' }) + .getByRole('button', { name: 'Delete folder' }) + .click() + await expect(filesListPage.getRows()).toHaveCount(1) }) + + test('Cancelling the confirmation of the DELETE hotkey keeps the folder', async ({ page, filesListPage }) => { + await filesListPage.getFilesList().press('ArrowDown') + await expect(page).toHaveURL(/\/apps\/files\/files\/\d+/) + await expect(filesListPage.getRows()).toHaveCount(2) + + await filesListPage.getFilesList().press('Delete') + + const dialog = page.getByRole('dialog', { name: 'Confirm deletion' }) + await dialog.getByRole('button', { name: 'Cancel' }).click() + + await expect(dialog).toBeHidden() + await expect(filesListPage.getRows()).toHaveCount(2) + }) })