[full-ci] Purge confirmation modal for deletion from files list (#9527)

* Purge confirmation modal for deletion from files list * Don't toggle toggleModalConfirmButton n times * Remove modal store actions, as modal won't be displayed anymore * Lint * Fix unit tests * Fix e2e tests * fix acceptance tests+ * Add notifications * Update packages/web-app-files/src/composables/actions/files/useFileActionsDelete.ts Co-authored-by: Dominik Schmidt <[email protected]> * Add missing import * add changelog item# * Fix acceptance tests * don't use gettext twice --------- Co-authored-by: Dominik Schmidt <[email protected]>
owncloud · Aug 3, 2023 · 17ec5c2 · 17ec5c2
1 parent d7a7089
commit 17ec5c2
Show file tree

Hide file tree

Showing 11 changed files with 94 additions and 106 deletions.
diff --git a/changelog/unreleased/enhancement-dont-display-confirmation-on-deletion 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
@@ -22,18 +22,27 @@ export const useFileActionsDelete = ({ store }: { store?: Store<any> } = {}) =>
   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<any> } = {}) =>
 
         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<any> } = {}) =>
       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
@@ -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<any> })
     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<any> })
 
     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<any> })
       )
     }
 
-    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<any> })
 
     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<any> })
 
   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<any> })
     })
   }
 
-  const filesList_delete = () => {
+  const filesList_delete = (resources: Resource[]) => {
+    resourcesToDelete.value = [...resources]
+
     const resourceSpaceMapping: Record<string, { space: SpaceResource; resources: Resource[] }> =
       unref(resources).reduce((acc, resource) => {
         if (resource.storageId in acc) {
@@ -202,9 +183,6 @@ export const useFileActionsDeleteResources = ({ store }: { store?: Store<any> })
                 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<any> })
     )
   }
 
-  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<any> })
         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
@@ -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
@@ -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<RouteLocation>({ name: routeName }) }),
     space: { driveType: 'personal', spaceRoles: { viewer: [], editor: [], manager: [] } }

diff --git a/...eb-app-files/tests/unit/composables/actions/helpers/useFileActionsDeleteResources.spec.ts b/...eb-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
@@ -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
@@ -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
@@ -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