From 0c9f3aa702ec7ee59835882eb4134c0d0dbb2baa Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sat, 22 Feb 2025 13:25:18 +0100 Subject: [PATCH] feat(files): allow to ignore warning to change file type * Missing pieces of https://github.com/nextcloud/server/issues/46528 * Add checkbox to not show this dialog again * Add user config as suggested by designers in files settings to reenable or diable this behavior. * Fix behavior of dialog: It says "keep .ext" but it does not keep the extension but cancels the operation. From the button label the user expects that the operation is continued but with the old extension. * Added more test coverage by adding component tests. Signed-off-by: Ferdinand Thiessen --- apps/files/lib/Service/UserConfig.php | 6 + .../components/FileEntry/FileEntryName.vue | 13 +- apps/files/src/eventbus.d.ts | 1 + apps/files/src/store/renaming.ts | 307 ++++++++---------- apps/files/src/store/userconfig.ts | 2 + apps/files/src/types.ts | 1 + .../views/DialogConfirmFileExtension.cy.ts | 161 +++++++++ .../src/views/DialogConfirmFileExtension.vue | 92 ++++++ apps/files/src/views/Settings.vue | 9 + cypress/e2e/files/files-renaming.cy.ts | 43 ++- 10 files changed, 455 insertions(+), 180 deletions(-) create mode 100644 apps/files/src/views/DialogConfirmFileExtension.cy.ts create mode 100644 apps/files/src/views/DialogConfirmFileExtension.vue diff --git a/apps/files/lib/Service/UserConfig.php b/apps/files/lib/Service/UserConfig.php index 0abcfb2a6add7..b01caf9f9608d 100644 --- a/apps/files/lib/Service/UserConfig.php +++ b/apps/files/lib/Service/UserConfig.php @@ -18,6 +18,12 @@ class UserConfig { 'default' => true, 'allowed' => [true, false], ], + [ + // Whether to show the "confirm file extension change" warning + 'key' => 'show_dialog_file_extension', + 'default' => true, + 'allowed' => [true, false], + ], [ // Whether to show the hidden files or not in the files list 'key' => 'show_hidden', diff --git a/apps/files/src/components/FileEntry/FileEntryName.vue b/apps/files/src/components/FileEntry/FileEntryName.vue index bc6b61ad54ca3..2fec9e5d556ac 100644 --- a/apps/files/src/components/FileEntry/FileEntryName.vue +++ b/apps/files/src/components/FileEntry/FileEntryName.vue @@ -23,7 +23,6 @@ @@ -117,11 +116,11 @@ export default defineComponent({ return this.isRenaming && this.filesListWidth < 512 }, newName: { - get() { - return this.renamingStore.newName + get(): string { + return this.renamingStore.newNodeName }, - set(newName) { - this.renamingStore.newName = newName + set(newName: string) { + this.renamingStore.newNodeName = newName }, }, @@ -249,7 +248,9 @@ export default defineComponent({ try { const status = await this.renamingStore.rename() if (status) { - showSuccess(t('files', 'Renamed "{oldName}" to "{newName}"', { oldName, newName })) + showSuccess( + t('files', 'Renamed "{oldName}" to "{newName}"', { oldName, newName: this.source.basename }), + ) this.$nextTick(() => { const nameContainer = this.$refs.basename as HTMLElement | undefined nameContainer?.focus() diff --git a/apps/files/src/eventbus.d.ts b/apps/files/src/eventbus.d.ts index ce0611580fe13..49ac67fe4db53 100644 --- a/apps/files/src/eventbus.d.ts +++ b/apps/files/src/eventbus.d.ts @@ -16,6 +16,7 @@ declare module '@nextcloud/event-bus' { 'files:node:created': Node 'files:node:deleted': Node 'files:node:updated': Node + 'files:node:rename': Node 'files:node:renamed': Node 'files:node:moved': { node: Node, oldSource: string } diff --git a/apps/files/src/store/renaming.ts b/apps/files/src/store/renaming.ts index 8ed455a182a8d..2ac9e06ba1617 100644 --- a/apps/files/src/store/renaming.ts +++ b/apps/files/src/store/renaming.ts @@ -3,184 +3,165 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ import type { Node } from '@nextcloud/files' -import type { RenamingStore } from '../types' import axios, { isAxiosError } from '@nextcloud/axios' import { emit, subscribe } from '@nextcloud/event-bus' import { FileType, NodeStatus } from '@nextcloud/files' -import { DialogBuilder } from '@nextcloud/dialogs' import { t } from '@nextcloud/l10n' +import { spawnDialog } from '@nextcloud/vue/functions/dialog' import { basename, dirname, extname } from 'path' import { defineStore } from 'pinia' import logger from '../logger' -import Vue from 'vue' -import IconCancel from '@mdi/svg/svg/cancel.svg?raw' -import IconCheck from '@mdi/svg/svg/check.svg?raw' - -let isDialogVisible = false - -const showWarningDialog = (oldExtension: string, newExtension: string): Promise => { - if (isDialogVisible) { - return Promise.resolve(false) - } - - isDialogVisible = true - - let message - - if (!oldExtension && newExtension) { - message = t( - 'files', - 'Adding the file extension "{new}" may render the file unreadable.', - { new: newExtension }, - ) - } else if (!newExtension) { - message = t( - 'files', - 'Removing the file extension "{old}" may render the file unreadable.', - { old: oldExtension }, - ) - } else { - message = t( - 'files', - 'Changing the file extension from "{old}" to "{new}" may render the file unreadable.', - { old: oldExtension, new: newExtension }, - ) - } - - return new Promise((resolve) => { - const dialog = new DialogBuilder() - .setName(t('files', 'Change file extension')) - .setText(message) - .setButtons([ - { - label: t('files', 'Keep {oldextension}', { oldextension: oldExtension }), - icon: IconCancel, - type: 'secondary', - callback: () => { - isDialogVisible = false - resolve(false) - }, +import Vue, { defineAsyncComponent, ref } from 'vue' +import { useUserConfigStore } from './userconfig' + +export const useRenamingStore = defineStore('renaming', () => { + /** + * The currently renamed node + */ + const renamingNode = ref() + /** + * The new name of the currently renamed node + */ + const newNodeName = ref('') + + /** + * Internal flag to only allow calling `rename` once. + */ + const isRenaming = ref(false) + + /** + * Execute the renaming. + * This will rename the node set as `renamingNode` to the configured new name `newName`. + * + * @return true if success, false if skipped (e.g. new and old name are the same) + * @throws Error if renaming fails, details are set in the error message + */ + async function rename(): Promise { + if (renamingNode.value === undefined) { + throw new Error('No node is currently being renamed') + } + + // Only rename once so we use this as some kind of mutex + if (isRenaming.value) { + return false + } + isRenaming.value = true + + const node = renamingNode.value + Vue.set(node, 'status', NodeStatus.LOADING) + + const userConfig = useUserConfigStore() + + let newName = newNodeName.value.trim() + const oldName = node.basename + const oldExtension = extname(oldName) + const newExtension = extname(newName) + // Check for extension change for files + if (node.type === FileType.File + && oldExtension !== newExtension + && userConfig.userConfig.show_dialog_file_extension + && !(await showFileExtensionDialog(oldExtension, newExtension)) + ) { + // user selected to use the old extension + newName = basename(newName, newExtension) + oldExtension + } + + const oldEncodedSource = node.encodedSource + try { + if (oldName === newName) { + return false + } + + // rename the node + node.rename(newName) + logger.debug('Moving file to', { destination: node.encodedSource, oldEncodedSource }) + // create MOVE request + await axios({ + method: 'MOVE', + url: oldEncodedSource, + headers: { + Destination: node.encodedSource, + Overwrite: 'F', }, - { - label: newExtension.length ? t('files', 'Use {newextension}', { newextension: newExtension }) : t('files', 'Remove extension'), - icon: IconCheck, - type: 'primary', - callback: () => { - isDialogVisible = false - resolve(true) - }, - }, - ]) - .build() - - dialog.show().then(() => { - dialog.hide() - }) - }) -} - -export const useRenamingStore = function(...args) { - const store = defineStore('renaming', { - state: () => ({ - renamingNode: undefined, - newName: '', - } as RenamingStore), - - actions: { - /** - * Execute the renaming. - * This will rename the node set as `renamingNode` to the configured new name `newName`. - * @return true if success, false if skipped (e.g. new and old name are the same) - * @throws Error if renaming fails, details are set in the error message - */ - async rename(): Promise { - if (this.renamingNode === undefined) { - throw new Error('No node is currently being renamed') - } - - const newName = this.newName.trim?.() || '' - const oldName = this.renamingNode.basename - const oldEncodedSource = this.renamingNode.encodedSource - - // Check for extension change for files - const oldExtension = extname(oldName) - const newExtension = extname(newName) - if (oldExtension !== newExtension && this.renamingNode.type === FileType.File) { - const proceed = await showWarningDialog(oldExtension, newExtension) - if (!proceed) { - return false - } + }) + + // Success 🎉 + emit('files:node:updated', node) + emit('files:node:renamed', node) + emit('files:node:moved', { + node, + oldSource: `${dirname(node.source)}/${oldName}`, + }) + + // Reset the state not changed + if (renamingNode.value === node) { + $reset() + } + + return true + } catch (error) { + logger.error('Error while renaming file', { error }) + // Rename back as it failed + node.rename(oldName) + if (isAxiosError(error)) { + // TODO: 409 means current folder does not exist, redirect ? + if (error?.response?.status === 404) { + throw new Error(t('files', 'Could not rename "{oldName}", it does not exist any more', { oldName })) + } else if (error?.response?.status === 412) { + throw new Error(t( + 'files', + 'The name "{newName}" is already used in the folder "{dir}". Please choose a different name.', + { + newName, + dir: basename(renamingNode.value!.dirname), + }, + )) } + } + // Unknown error + throw new Error(t('files', 'Could not rename "{oldName}"', { oldName })) + } finally { + Vue.set(node, 'status', undefined) + isRenaming.value = false + } + } - if (oldName === newName) { - return false - } + /** + * Reset the store state + */ + function $reset(): void { + newNodeName.value = '' + renamingNode.value = undefined + } - const node = this.renamingNode - Vue.set(node, 'status', NodeStatus.LOADING) - - try { - // rename the node - this.renamingNode.rename(newName) - logger.debug('Moving file to', { destination: this.renamingNode.encodedSource, oldEncodedSource }) - // create MOVE request - await axios({ - method: 'MOVE', - url: oldEncodedSource, - headers: { - Destination: this.renamingNode.encodedSource, - Overwrite: 'F', - }, - }) - - // Success 🎉 - emit('files:node:updated', this.renamingNode as Node) - emit('files:node:renamed', this.renamingNode as Node) - emit('files:node:moved', { - node: this.renamingNode as Node, - oldSource: `${dirname(this.renamingNode.source)}/${oldName}`, - }) - this.$reset() - return true - } catch (error) { - logger.error('Error while renaming file', { error }) - // Rename back as it failed - this.renamingNode.rename(oldName) - if (isAxiosError(error)) { - // TODO: 409 means current folder does not exist, redirect ? - if (error?.response?.status === 404) { - throw new Error(t('files', 'Could not rename "{oldName}", it does not exist any more', { oldName })) - } else if (error?.response?.status === 412) { - throw new Error(t( - 'files', - 'The name "{newName}" is already used in the folder "{dir}". Please choose a different name.', - { - newName, - dir: basename(this.renamingNode.dirname), - }, - )) - } - } - // Unknown error - throw new Error(t('files', 'Could not rename "{oldName}"', { oldName })) - } finally { - Vue.set(node, 'status', undefined) - } - }, - }, + // Make sure we only register the listeners once + subscribe('files:node:rename', (node: Node) => { + renamingNode.value = node + newNodeName.value = node.basename }) - const renamingStore = store(...args) + return { + $reset, - // Make sure we only register the listeners once - if (!renamingStore._initialized) { - subscribe('files:node:rename', function(node: Node) { - renamingStore.renamingNode = node - renamingStore.newName = node.basename - }) - renamingStore._initialized = true + newNodeName, + rename, + renamingNode, } +}) - return renamingStore +/** + * Show a dialog asking user for confirmation about changing the file extension. + * + * @param oldExtension the old file name extension + * @param newExtension the new file name extension + */ +async function showFileExtensionDialog(oldExtension: string, newExtension: string): Promise { + const { promise, resolve } = Promise.withResolvers() + spawnDialog( + defineAsyncComponent(() => import('../views/DialogConfirmFileExtension.vue')), + { oldExtension, newExtension }, + (useNewExtension: unknown) => resolve(Boolean(useNewExtension)), + ) + return await promise } diff --git a/apps/files/src/store/userconfig.ts b/apps/files/src/store/userconfig.ts index 95829c849e852..d84a5e0d93554 100644 --- a/apps/files/src/store/userconfig.ts +++ b/apps/files/src/store/userconfig.ts @@ -17,6 +17,8 @@ const initialUserConfig = loadState('files', 'config', { sort_favorites_first: true, sort_folders_first: true, grid_view: false, + + show_dialog_file_extension: true, }) export const useUserConfigStore = defineStore('userconfig', () => { diff --git a/apps/files/src/types.ts b/apps/files/src/types.ts index 62ba53b89d277..4bf8a557f4959 100644 --- a/apps/files/src/types.ts +++ b/apps/files/src/types.ts @@ -52,6 +52,7 @@ export interface PathOptions { export interface UserConfig { [key: string]: boolean|undefined + show_dialog_file_extension: boolean, show_hidden: boolean crop_image_previews: boolean sort_favorites_first: boolean diff --git a/apps/files/src/views/DialogConfirmFileExtension.cy.ts b/apps/files/src/views/DialogConfirmFileExtension.cy.ts new file mode 100644 index 0000000000000..460497dd91fc4 --- /dev/null +++ b/apps/files/src/views/DialogConfirmFileExtension.cy.ts @@ -0,0 +1,161 @@ +/*! + * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +import { createTestingPinia } from '@pinia/testing' +import DialogConfirmFileExtension from './DialogConfirmFileExtension.vue' +import { useUserConfigStore } from '../store/userconfig' + +describe('DialogConfirmFileExtension', () => { + it('renders with both extensions', () => { + cy.mount(DialogConfirmFileExtension, { + propsData: { + oldExtension: '.old', + newExtension: '.new', + }, + global: { + plugins: [createTestingPinia({ + createSpy: cy.spy, + })], + }, + }) + + cy.findByRole('dialog') + .as('dialog') + .should('be.visible') + cy.get('@dialog') + .findByRole('heading') + .should('contain.text', 'Change file extension') + cy.get('@dialog') + .findByRole('checkbox', { name: /Do not show this dialog again/i }) + .should('exist') + .and('not.be.checked') + cy.get('@dialog') + .findByRole('button', { name: 'Keep .old' }) + .should('be.visible') + cy.get('@dialog') + .findByRole('button', { name: 'Use .new' }) + .should('be.visible') + }) + + it('renders without old extension', () => { + cy.mount(DialogConfirmFileExtension, { + propsData: { + newExtension: '.new', + }, + global: { + plugins: [createTestingPinia({ + createSpy: cy.spy, + })], + }, + }) + + cy.findByRole('dialog') + .as('dialog') + .should('be.visible') + cy.get('@dialog') + .findByRole('button', { name: 'Keep without extension' }) + .should('be.visible') + cy.get('@dialog') + .findByRole('button', { name: 'Use .new' }) + .should('be.visible') + }) + + it('renders without new extension', () => { + cy.mount(DialogConfirmFileExtension, { + propsData: { + oldExtension: '.old', + }, + global: { + plugins: [createTestingPinia({ + createSpy: cy.spy, + })], + }, + }) + + cy.findByRole('dialog') + .as('dialog') + .should('be.visible') + cy.get('@dialog') + .findByRole('button', { name: 'Keep .old' }) + .should('be.visible') + cy.get('@dialog') + .findByRole('button', { name: 'Remove extension' }) + .should('be.visible') + }) + + it('emits correct value on keep old', () => { + cy.mount(DialogConfirmFileExtension, { + propsData: { + oldExtension: '.old', + newExtension: '.new', + }, + global: { + plugins: [createTestingPinia({ + createSpy: cy.spy, + })], + }, + }).as('component') + + cy.findByRole('dialog') + .as('dialog') + .should('be.visible') + cy.get('@dialog') + .findByRole('button', { name: 'Keep .old' }) + .click() + cy.get('@component') + .its('wrapper') + .should((wrapper) => expect(wrapper.emitted('close')).to.eql([[false]])) + }) + + it('emits correct value on use new', () => { + cy.mount(DialogConfirmFileExtension, { + propsData: { + oldExtension: '.old', + newExtension: '.new', + }, + global: { + plugins: [createTestingPinia({ + createSpy: cy.spy, + })], + }, + }).as('component') + + cy.findByRole('dialog') + .as('dialog') + .should('be.visible') + cy.get('@dialog') + .findByRole('button', { name: 'Use .new' }) + .click() + cy.get('@component') + .its('wrapper') + .should((wrapper) => expect(wrapper.emitted('close')).to.eql([[true]])) + }) + + it('updates user config when checking the checkbox', () => { + const pinia = createTestingPinia({ + createSpy: cy.spy, + }) + + cy.mount(DialogConfirmFileExtension, { + propsData: { + oldExtension: '.old', + newExtension: '.new', + }, + global: { + plugins: [pinia], + }, + }).as('component') + + cy.findByRole('dialog') + .as('dialog') + .should('be.visible') + cy.get('@dialog') + .findByRole('checkbox', { name: /Do not show this dialog again/i }) + .check({ force: true }) + + cy.wrap(useUserConfigStore()) + .its('update') + .should('have.been.calledWith', 'show_dialog_file_extension', false) + }) +}) diff --git a/apps/files/src/views/DialogConfirmFileExtension.vue b/apps/files/src/views/DialogConfirmFileExtension.vue new file mode 100644 index 0000000000000..cc1ee363f9807 --- /dev/null +++ b/apps/files/src/views/DialogConfirmFileExtension.vue @@ -0,0 +1,92 @@ + + + + + + + diff --git a/apps/files/src/views/Settings.vue b/apps/files/src/views/Settings.vue index 1def0e41c1566..8a35f93830b8d 100644 --- a/apps/files/src/views/Settings.vue +++ b/apps/files/src/views/Settings.vue @@ -83,6 +83,15 @@ + + {{ t('files', 'Prevent warning dialogs from open or reenable them.') }} + + {{ t('files', 'Warn about changed file extension.') }} + + + {{ t('files', 'Speed up your Files experience with these quick shortcuts.') }} diff --git a/cypress/e2e/files/files-renaming.cy.ts b/cypress/e2e/files/files-renaming.cy.ts index 0e6984321025d..269f0bd252b45 100644 --- a/cypress/e2e/files/files-renaming.cy.ts +++ b/cypress/e2e/files/files-renaming.cy.ts @@ -74,7 +74,7 @@ describe('files: Rename nodes', { testIsolation: true }, () => { }) it('shows accessible loading information', () => { - const { resolve, promise } = Promise.withResolvers() + const { resolve, promise } = Promise.withResolvers() getRowForFile('file.txt').should('be.visible') @@ -106,7 +106,7 @@ describe('files: Rename nodes', { testIsolation: true }, () => { .should('not.exist') cy.log('Resolve promise to preoceed with MOVE request') - .then(() => resolve(null)) + .then(() => resolve()) // Ensure the request is done (file renamed) cy.wait('@moveFile') @@ -194,7 +194,7 @@ describe('files: Rename nodes', { testIsolation: true }, () => { .should('not.exist') }) - it('shows warning on extension change', () => { + it('shows warning on extension change - select new extension', () => { getRowForFile('file.txt').should('be.visible') triggerActionForFile('file.txt', 'rename') @@ -202,39 +202,60 @@ describe('files: Rename nodes', { testIsolation: true }, () => { .findByRole('textbox', { name: 'Filename' }) .should('be.visible') .type('{selectAll}file.md') - .should(haveValidity('')) .type('{enter}') // See warning dialog cy.findByRole('dialog', { name: 'Change file extension' }) .should('be.visible') - .findByRole('button', { name: /use/i }) + .findByRole('button', { name: 'Use .md' }) .click() // See it is renamed getRowForFile('file.md').should('be.visible') }) - it('shows warning on extension change and allow cancellation', () => { + it('shows warning on extension change - select old extension', () => { getRowForFile('file.txt').should('be.visible') triggerActionForFile('file.txt', 'rename') getRowForFile('file.txt') .findByRole('textbox', { name: 'Filename' }) .should('be.visible') - .type('{selectAll}file.md') - .should(haveValidity('')) + .type('{selectAll}document.md') .type('{enter}') // See warning dialog cy.findByRole('dialog', { name: 'Change file extension' }) .should('be.visible') - .findByRole('button', { name: /keep/i }) + .findByRole('button', { name: 'Keep .txt' }) .click() - // See it is not renamed + // See it is renamed + getRowForFile('document.txt').should('be.visible') + }) + + it('shows warning on extension removal', () => { getRowForFile('file.txt').should('be.visible') - getRowForFile('file.md').should('not.exist') + + triggerActionForFile('file.txt', 'rename') + getRowForFile('file.txt') + .findByRole('textbox', { name: 'Filename' }) + .should('be.visible') + .type('{selectAll}file') + .type('{enter}') + + cy.findByRole('dialog', { name: 'Change file extension' }) + .should('be.visible') + .findByRole('button', { name: 'Keep .txt' }) + .should('be.visible') + cy.findByRole('dialog', { name: 'Change file extension' }) + .findByRole('button', { name: 'Remove extension' }) + .should('be.visible') + .click() + + // See it is renamed + getRowForFile('file').should('be.visible') + getRowForFile('file.txt').should('not.exist') }) it('does not show warning on folder renaming with a dot', () => {