diff --git a/changelog/unreleased/enhancement-dont-display-confirmation-on-deletion b/changelog/unreleased/enhancement-dont-display-confirmation-on-deletion new file mode 100644 index 00000000000..ccebbf1093a --- /dev/null +++ b/changelog/unreleased/enhancement-dont-display-confirmation-on-deletion @@ -0,0 +1,9 @@ +Enhancement: Don't display confirmation dialog on file deletion + +We've removed the confirmation dialog while deletion files or folder to enhance the user experience. +We also show success notifications after the operation. + +This doesn't have impact on the trash bin, confirmation dialog will still be displayed there. + +https://github.com/owncloud/web/pull/9527 +https://github.com/owncloud/web/issues/5974 diff --git a/packages/web-app-files/src/composables/actions/files/useFileActionsDelete.ts b/packages/web-app-files/src/composables/actions/files/useFileActionsDelete.ts index 5d1549f04d3..b59c4ba438c 100644 --- a/packages/web-app-files/src/composables/actions/files/useFileActionsDelete.ts +++ b/packages/web-app-files/src/composables/actions/files/useFileActionsDelete.ts @@ -22,18 +22,27 @@ export const useFileActionsDelete = ({ store }: { store?: Store } = {}) => const router = useRouter() const hasSpaces = useCapabilitySpacesEnabled() const hasPermanentDeletion = useCapabilityFilesPermanentDeletion() - const { displayDialog } = useFileActionsDeleteResources({ store }) + const { displayDialog, filesList_delete } = useFileActionsDeleteResources({ store }) const { $gettext } = useGettext() - const handler = ({ space, resources }: FileActionOptions) => { + const handler = ({ + space, + resources, + deletePermanent + }: FileActionOptions & { deletePermanent: boolean }) => { if (isLocationCommonActive(router, 'files-common-search')) { resources = resources.filter( (r) => r.canBeDeleted() && (!unref(hasSpaces) || !r.isShareRoot()) && !isProjectSpaceResource(r) ) } - displayDialog(space, resources) + if (deletePermanent) { + displayDialog(space, resources) + return + } + + filesList_delete(resources) } const actions = computed((): FileAction[] => [ @@ -55,7 +64,7 @@ export const useFileActionsDelete = ({ store }: { store?: Store } = {}) => return deleteLabel }, - handler, + handler: ({ space, resources }) => handler({ space, resources, deletePermanent: false }), isEnabled: ({ space, resources }) => { if ( !isLocationSpacesActive(router, 'files-spaces-generic') && @@ -99,7 +108,7 @@ export const useFileActionsDelete = ({ store }: { store?: Store } = {}) => name: 'delete-permanent', icon: 'delete-bin-5', label: () => $gettext('Delete'), - handler, + handler: ({ space, resources }) => handler({ space, resources, deletePermanent: true }), isEnabled: ({ space, resources }) => { if (!isLocationTrashActive(router, 'files-trash-generic')) { return false diff --git a/packages/web-app-files/src/composables/actions/helpers/useFileActionsDeleteResources.ts b/packages/web-app-files/src/composables/actions/helpers/useFileActionsDeleteResources.ts index 63b7d3516e4..042ae26b94a 100644 --- a/packages/web-app-files/src/composables/actions/helpers/useFileActionsDeleteResources.ts +++ b/packages/web-app-files/src/composables/actions/helpers/useFileActionsDeleteResources.ts @@ -4,7 +4,7 @@ import { isSameResource } from '../../../helpers/resource' import { buildWebDavFilesTrashPath } from '../../../helpers/resources' import { buildWebDavSpacesTrashPath, Resource, SpaceResource } from 'web-client/src/helpers' import PQueue from 'p-queue' -import { isLocationTrashActive, isLocationSpacesActive } from '../../../router' +import { isLocationSpacesActive } from '../../../router' import { dirname } from 'path' import { createFileRouteOptions } from 'web-pkg/src/helpers/router' import { computed, unref } from 'vue' @@ -48,10 +48,6 @@ export const useFileActionsDeleteResources = ({ store }: { store?: Store }) return parseInt(queryItemAsString(unref(itemsPerPageQuery))) }) - const isInTrashbin = computed(() => { - return isLocationTrashActive(router, 'files-trash-generic') - }) - const resources = computed(() => { return cloneStateObject(unref(resourcesToDelete)) }) @@ -63,13 +59,9 @@ export const useFileActionsDeleteResources = ({ store }: { store?: Store }) if (currentResources.length === 1) { if (isFolder) { - title = unref(isInTrashbin) - ? $gettext('Permanently delete folder %{name}') - : $gettext('Delete folder %{name}') + title = $gettext('Permanently delete folder %{name}') } else { - title = unref(isInTrashbin) - ? $gettext('Permanently delete file %{name}') - : $gettext('Delete file %{name}') + title = $gettext('Permanently delete file %{name}') } return $gettextInterpolate( title, @@ -80,17 +72,11 @@ export const useFileActionsDeleteResources = ({ store }: { store?: Store }) ) } - title = unref(isInTrashbin) - ? $ngettext( - 'Permanently delete selected resource?', - 'Permanently delete %{amount} selected resources?', - currentResources.length - ) - : $ngettext( - 'Delete selected resource?', - 'Delete %{amount} selected resources?', - currentResources.length - ) + title = $ngettext( + 'Permanently delete selected resource?', + 'Permanently delete %{amount} selected resources?', + currentResources.length + ) return $gettextInterpolate(title, { amount: currentResources.length }, false) }) @@ -101,24 +87,18 @@ export const useFileActionsDeleteResources = ({ store }: { store?: Store }) if (currentResources.length === 1) { if (isFolder) { - return unref(isInTrashbin) - ? $gettext( - 'Are you sure you want to delete this folder? All its content will be permanently removed. This action cannot be undone.' - ) - : $gettext('Are you sure you want to delete this folder?') + return $gettext( + 'Are you sure you want to delete this folder? All its content will be permanently removed. This action cannot be undone.' + ) } - return unref(isInTrashbin) - ? $gettext( - 'Are you sure you want to delete this file? All its content will be permanently removed. This action cannot be undone.' - ) - : $gettext('Are you sure you want to delete this file?') + return $gettext( + 'Are you sure you want to delete this file? All its content will be permanently removed. This action cannot be undone.' + ) } - return unref(isInTrashbin) - ? $gettext( - 'Are you sure you want to delete all selected resources? All their content will be permanently removed. This action cannot be undone.' - ) - : $gettext('Are you sure you want to delete all selected resources?') + return $gettext( + 'Are you sure you want to delete all selected resources? All their content will be permanently removed. This action cannot be undone.' + ) }) const trashbin_deleteOp = (space, resource) => { @@ -156,7 +136,6 @@ export const useFileActionsDeleteResources = ({ store }: { store?: Store }) const trashbin_delete = (space: SpaceResource) => { // TODO: use clear all if all files are selected - store.dispatch('toggleModalConfirmButton') // FIXME: Implement proper batch delete and add loading indicator for (const file of unref(resources)) { const p = queue.add(() => { @@ -171,7 +150,9 @@ export const useFileActionsDeleteResources = ({ store }: { store?: Store }) }) } - const filesList_delete = () => { + const filesList_delete = (resources: Resource[]) => { + resourcesToDelete.value = [...resources] + const resourceSpaceMapping: Record = unref(resources).reduce((acc, resource) => { if (resource.storageId in acc) { @@ -202,9 +183,6 @@ export const useFileActionsDeleteResources = ({ store }: { store?: Store }) loadingCallbackArgs }) .then(async () => { - store.dispatch('hideModal') - store.dispatch('toggleModalConfirmButton') - // Load quota if ( isLocationSpacesActive(router, 'files-spaces-generic') && @@ -253,11 +231,6 @@ export const useFileActionsDeleteResources = ({ store }: { store?: Store }) ) } - const deleteHelper = (space: SpaceResource) => { - store.dispatch('toggleModalConfirmButton') - unref(isInTrashbin) ? trashbin_delete(space) : filesList_delete() - } - const displayDialog = (space: SpaceResource, resources: Resource[]) => { resourcesToDelete.value = [...resources] @@ -271,7 +244,7 @@ export const useFileActionsDeleteResources = ({ store }: { store?: Store }) store.dispatch('hideModal') }, onConfirm: () => { - deleteHelper(space) + trashbin_delete(space) } } diff --git a/packages/web-app-files/src/store/actions.ts b/packages/web-app-files/src/store/actions.ts index 41a74c02b6d..b90527f6933 100644 --- a/packages/web-app-files/src/store/actions.ts +++ b/packages/web-app-files/src/store/actions.ts @@ -169,7 +169,7 @@ export default { ) { const { $gettext, - interpolate: $gettextInterpolate, + $ngettext, space, files, clientService, @@ -186,7 +186,7 @@ export default { removedFiles.push(file) }) .catch((error) => { - let translated = $gettext('Failed to delete "%{file}"') + let title = $gettext('Failed to delete "%{file}"', { file: file.name }) if (error.statusCode === 423) { if (firstRun) { return context.dispatch('deleteFiles', { @@ -198,13 +198,12 @@ export default { }) } - translated = $gettext('Failed to delete "%{file}" - the file is locked') + title = $gettext('Failed to delete "%{file}" - the file is locked', { file: file.name }) } - const title = $gettextInterpolate(translated, { file: file.name }, true) context.dispatch( 'showErrorMessage', { - title: title, + title, error }, { root: true } @@ -219,6 +218,20 @@ export default { context.commit('REMOVE_FILES', removedFiles) context.commit('REMOVE_FILES_FROM_SEARCHED', removedFiles) context.commit('RESET_SELECTION') + + if (removedFiles.length) { + const title = + removedFiles.length === 1 && files.length === 1 + ? $gettext('"%{item}" was moved to trash bin', { item: removedFiles[0].name }) + : $ngettext( + '%{itemCount} item was moved to trash bin', + '%{itemCount} items were moved to trash bin', + removedFiles.length, + { itemCount: removedFiles.length.toString() }, + true + ) + context.dispatch('showMessage', { title }, { root: true }) + } }) }, clearTrashBin(context) { diff --git a/packages/web-app-files/tests/unit/composables/actions/files/useFileActionsDelete.spec.ts b/packages/web-app-files/tests/unit/composables/actions/files/useFileActionsDelete.spec.ts index f74b523800c..8ade25c7d7c 100644 --- a/packages/web-app-files/tests/unit/composables/actions/files/useFileActionsDelete.spec.ts +++ b/packages/web-app-files/tests/unit/composables/actions/files/useFileActionsDelete.spec.ts @@ -143,19 +143,18 @@ describe('delete', () => { deletableResourceIds: ['1', '2', '3'] } ])('should filter non deletable resources', ({ resources, deletableResourceIds }) => { - const displayDialogMock = jest.fn() + const filesListDeleteMock = jest.fn() const { wrapper } = getWrapper({ searchLocation: true, - displayDialogMock, + filesListDeleteMock, setup: () => { const store = useStore() const { actions } = useFileActionsDelete({ store }) unref(actions)[0].handler({ space: null, resources }) - expect(displayDialogMock).toHaveBeenCalledWith( - null, + expect(filesListDeleteMock).toHaveBeenCalledWith( resources.filter((r) => deletableResourceIds.includes(r.id as string)) ) } @@ -170,7 +169,7 @@ function getWrapper({ deletePermanent = false, invalidLocation = false, searchLocation = false, - displayDialogMock = jest.fn(), + filesListDeleteMock = jest.fn(), setup = () => undefined } = {}) { const routeName = invalidLocation @@ -182,7 +181,7 @@ function getWrapper({ : 'files-spaces-generic' jest .mocked(useFileActionsDeleteResources) - .mockImplementation(() => ({ displayDialog: displayDialogMock, filesList_delete: jest.fn() })) + .mockImplementation(() => ({ filesList_delete: filesListDeleteMock, displayDialog: jest.fn() })) const mocks = { ...defaultComponentMocks({ currentRoute: mock({ name: routeName }) }), space: { driveType: 'personal', spaceRoles: { viewer: [], editor: [], manager: [] } } diff --git a/packages/web-app-files/tests/unit/composables/actions/helpers/useFileActionsDeleteResources.spec.ts b/packages/web-app-files/tests/unit/composables/actions/helpers/useFileActionsDeleteResources.spec.ts index d5b65743bc8..0f1a38d3f21 100644 --- a/packages/web-app-files/tests/unit/composables/actions/helpers/useFileActionsDeleteResources.spec.ts +++ b/packages/web-app-files/tests/unit/composables/actions/helpers/useFileActionsDeleteResources.spec.ts @@ -21,11 +21,9 @@ describe('deleteResources', () => { const { wrapper } = getWrapper({ currentFolder, setup: async ({ displayDialog, filesList_delete }, { space, router, storeOptions }) => { - displayDialog(space, [{ id: 2, path: '/folder/fileToDelete.txt' }]) - await filesList_delete() + await filesList_delete([{ id: 2, path: '/folder/fileToDelete.txt' }]) await nextTick() expect(router.push).toHaveBeenCalledTimes(0) - expect(storeOptions.actions.hideModal).toHaveBeenCalledTimes(1) } }) }) @@ -35,11 +33,9 @@ describe('deleteResources', () => { const { wrapper } = getWrapper({ currentFolder, setup: async ({ displayDialog, filesList_delete }, { space, router, storeOptions }) => { - displayDialog(space, resourcesToDelete) - await filesList_delete() + await filesList_delete(resourcesToDelete) await nextTick() expect(router.push).toHaveBeenCalledTimes(1) - expect(storeOptions.actions.hideModal).toHaveBeenCalledTimes(1) } }) }) diff --git a/tests/acceptance/features/webUITrashbinDelete/trashbinDelete.feature b/tests/acceptance/features/webUITrashbinDelete/trashbinDelete.feature index 1bcfa0cdb44..b2ecc756059 100644 --- a/tests/acceptance/features/webUITrashbinDelete/trashbinDelete.feature +++ b/tests/acceptance/features/webUITrashbinDelete/trashbinDelete.feature @@ -27,14 +27,14 @@ Feature: files and folders can be deleted from the trashbin @issue-product-188 Scenario: Delete folders from trashbin and check that they are gone - When the user deletes folder "simple-folder" using the webUI - And the user deletes folder "Folder,With,Comma" using the webUI + When the user deletes the file "simple-folder" from the deleted files list + And the user deletes the file "Folder,With,Comma" from the deleted files list Then folder "simple-folder" should not be listed on the webUI And folder "Folder,With,Comma" should not be listed on the webUI @ocisSmokeTest @issue-4582 Scenario: Select some files and delete from trashbin in a batch - When the user batch deletes these files using the webUI + When the user batch deletes these files permanently using the webUI | name | | lorem.txt | | lorem-big.txt | @@ -97,7 +97,7 @@ Feature: files and folders can be deleted from the trashbin | fo.xyz | And the user has browsed to the trashbin page And the user has reloaded the current page of the webUI - When the user batch deletes these files using the webUI + When the user batch deletes these files permanently using the webUI | name | | fo. | | fo.1 | @@ -113,7 +113,7 @@ Feature: files and folders can be deleted from the trashbin | name | | lorem.txt | | lorem-big.txt | - And the user batch deletes the marked files using the webUI + And the user batch deletes the marked files permanently using the webUI Then file "lorem.txt" should be listed on the webUI And file "lorem-big.txt" should be listed on the webUI But file "data.zip" should not be listed on the webUI @@ -122,5 +122,5 @@ Feature: files and folders can be deleted from the trashbin @issue-product-188 @issue-4582 Scenario: Select all files and delete from trashbin in a batch When the user marks all files for batch action using the webUI - And the user batch deletes the marked files using the webUI + And the user batch deletes the marked files permanently using the webUI Then there should be no resources listed on the webUI diff --git a/tests/acceptance/pageObjects/FilesPageElement/fileActionsMenu.js b/tests/acceptance/pageObjects/FilesPageElement/fileActionsMenu.js index 67749212760..ba86501ae50 100644 --- a/tests/acceptance/pageObjects/FilesPageElement/fileActionsMenu.js +++ b/tests/acceptance/pageObjects/FilesPageElement/fileActionsMenu.js @@ -67,7 +67,6 @@ module.exports = { */ delete: async function () { await this.performFileAction(this.FileAction.delete) - await this.api.page.FilesPageElement.filesList().confirmDeletion() return this }, /** diff --git a/tests/acceptance/pageObjects/personalPage.js b/tests/acceptance/pageObjects/personalPage.js index 455fe50213b..d02e5047a90 100644 --- a/tests/acceptance/pageObjects/personalPage.js +++ b/tests/acceptance/pageObjects/personalPage.js @@ -230,11 +230,7 @@ module.exports = { deleteAllCheckedFiles: function () { return this.waitForElementVisible('@deleteSelectedButton') .click('@deleteSelectedButton') - .waitForElementVisible('@dialog') - .waitForAnimationToFinish() // wait for transition on the modal to finish - .click('@dialogConfirmBtnEnabled') .waitForAjaxCallsToStartAndFinish() - .waitForElementNotPresent('@dialog') }, confirmFileOverwrite: async function () { await this.waitForAnimationToFinish() // wait for transition on the modal to finish diff --git a/tests/acceptance/stepDefinitions/filesContext.js b/tests/acceptance/stepDefinitions/filesContext.js index 994a8c1c76f..8a09bc0c3ab 100644 --- a/tests/acceptance/stepDefinitions/filesContext.js +++ b/tests/acceptance/stepDefinitions/filesContext.js @@ -482,7 +482,7 @@ When('the user marks all files for batch action using the webUI', function () { }) When('the user batch deletes the marked files permanently using the webUI', function () { - return client.page.trashBinPage().deleteSelectedPermanently() + return client.page.trashbinPage().deleteSelectedPermanently() }) When('the user/public batch deletes the marked files using the webUI', function () { @@ -505,7 +505,7 @@ When( await client.page.FilesPageElement.filesList().toggleFileOrFolderCheckbox('enable', item[0]) } - return client.page.trashBinPage().deleteSelectedPermanently() + return client.page.trashbinPage().deleteSelectedPermanently() } ) diff --git a/tests/e2e/support/objects/app-files/resource/actions.ts b/tests/e2e/support/objects/app-files/resource/actions.ts index a910562c952..aedcf7ca10b 100644 --- a/tests/e2e/support/objects/app-files/resource/actions.ts +++ b/tests/e2e/support/objects/app-files/resource/actions.ts @@ -949,15 +949,12 @@ export const deleteResource = async (args: deleteResourceArgs): Promise => await sidebar.open({ page, resource: resource.name }) await sidebar.openPanel({ page, name: 'actions' }) await page.locator(deleteButtonSidebar).first().click() - await Promise.all([ - page.waitForResponse( - (resp) => - resp.url().includes(encodeURIComponent(resource.name)) && - resp.status() === 204 && - resp.request().method() === 'DELETE' - ), - page.locator(util.format(actionConfirmationButton, 'Delete')).click() - ]) + await page.waitForResponse( + (resp) => + resp.url().includes(encodeURIComponent(resource.name)) && + resp.status() === 204 && + resp.request().method() === 'DELETE' + ) await sidebar.close({ page }) } break @@ -971,20 +968,17 @@ export const deleteResource = async (args: deleteResourceArgs): Promise => } await page.locator(deleteButtonBatchAction).click() - await Promise.all([ - page.waitForResponse((resp) => { - if (resp.status() === 204 && resp.request().method() === 'DELETE') { - deletetedResources.push(decodeURIComponent(resp.url().split('/').pop())) - } - // waiting for GET response after all the resource are deleted with batch action - return ( - resp.url().includes('graph/v1.0/drives') && - resp.status() === 200 && - resp.request().method() === 'GET' - ) - }), - page.locator(util.format(actionConfirmationButton, 'Delete')).click() - ]) + await page.waitForResponse((resp) => { + if (resp.status() === 204 && resp.request().method() === 'DELETE') { + deletetedResources.push(decodeURIComponent(resp.url().split('/').pop())) + } + // waiting for GET response after all the resource are deleted with batch action + return ( + resp.url().includes(config.ocis ? 'graph/v1.0/drives' : 'ocs/v1.php/cloud/users') && + resp.status() === 200 && + resp.request().method() === 'GET' + ) + }) // assertion that the resources actually got deleted expect(deletetedResources.length).toBe(resourcesWithInfo.length) for (const resource of resourcesWithInfo) {