[full-ci] Improve UX of the conflict dialog (#7983)

Improve UX of the conflict dialog

changelog/unreleased/enhancement-conflict-dialog-ux

@@ -0,0 +1,10 @@
+Enhancement: Conflict dialog UX
+
+The UX of the conflict dialog has been improved slightly:
+
+* The name of the conflicting resource is now written in quotes
+* The title of the dialog now tells the difference between files and folders
+* The "Skip"-dialog now tells the difference between files and folders
+
+https://github.com/owncloud/web/pull/7983
+https://github.com/owncloud/web/issues/7682

packages/web-app-files/src/helpers/resource/actions/transfer.ts

@@ -113,7 +113,6 @@ export class ResourceTransfer extends ConflictDialog {
 
     const resolvedConflicts = await this.resolveAllConflicts(
       this.resourcesToMove,
-      this.targetSpace,
       this.targetFolder,
       targetFolderResources
     )

packages/web-app-files/src/helpers/resource/actions/upload.ts

@@ -11,6 +11,11 @@ import { ConflictDialog, ResolveConflict, resolveFileNameDuplicate, ResolveStrat
 import { locationPublicLink } from '../../../router/public'
 import { locationSpacesGeneric } from '../../../router/spaces'
 
+interface ConflictedResource {
+  name: string
+  type: string
+}
+
 export class ResourcesUpload extends ConflictDialog {
   constructor(
     private filesToUpload: File[],
@@ -38,15 +43,24 @@ export class ResourcesUpload extends ConflictDialog {
     super(createModal, hideModal, showMessage, $gettext, $ngettext, $gettextInterpolate)
   }
 
-  async perform() {
-    const conflicts = []
+  perform() {
     const uppyResources: UppyResource[] = this.inputFilesToUppyFiles(this.filesToUpload)
     const quotaExceeded = this.checkQuotaExceeded(uppyResources)
-
     if (quotaExceeded) {
       return this.$uppyService.clearInputs()
     }
-    for (const file of uppyResources) {
+
+    const conflicts = this.getConflicts(uppyResources)
+    if (conflicts.length) {
+      return this.displayOverwriteDialog(uppyResources, conflicts)
+    }
+
+    this.handleUppyFileUpload(this.space, this.currentFolder, uppyResources)
+  }
+
+  getConflicts(files: UppyResource[]): ConflictedResource[] {
+    const conflicts: ConflictedResource[] = []
+    for (const file of files) {
       const relativeFilePath = file.meta.relativePath
       if (relativeFilePath) {
         // Logic for folders, applies to all files inside folder and subfolders
@@ -56,10 +70,7 @@ export class ResourcesUpload extends ConflictDialog {
           if (conflicts.some((conflict) => conflict.name === rootFolder)) {
             continue
           }
-          conflicts.push({
-            name: rootFolder,
-            type: 'folder'
-          })
+          conflicts.push({ name: rootFolder, type: 'folder' })
           continue
         }
       }
@@ -68,17 +79,10 @@ export class ResourcesUpload extends ConflictDialog {
         (f) => f.name === file.name && !file.meta.relativeFolder
       )
       if (exists) {
-        conflicts.push({
-          name: file.name,
-          type: 'file'
-        })
+        conflicts.push({ name: file.name, type: 'file' })
       }
     }
-    if (conflicts.length) {
-      await this.displayOverwriteDialog(uppyResources, conflicts)
-    } else {
-      this.handleUppyFileUpload(this.space, this.currentFolder, uppyResources)
-    }
+    return conflicts
   }
 
   async handleUppyFileUpload(space: SpaceResource, currentFolder: string, files: UppyResource[]) {
@@ -100,9 +104,9 @@ export class ResourcesUpload extends ConflictDialog {
     }
   }
 
-  async displayOverwriteDialog(files: UppyResource[], conflicts) {
-    let count = 0
-    const allConflictsCount = conflicts.length
+  async displayOverwriteDialog(files: UppyResource[], conflicts: ConflictedResource[]) {
+    let fileCount = 0
+    let folderCount = 0
     const resolvedFileConflicts = []
     const resolvedFolderConflicts = []
     let doForAllConflicts = false
@@ -128,14 +132,19 @@ export class ResourcesUpload extends ConflictDialog {
         })
         continue
       }
-      const conflictsLeft = allConflictsCount - count
+
+      const conflictsLeft =
+        conflicts.filter((c) => c.type === conflict.type).length -
+        (isFolder ? folderCount : fileCount)
+
       const resolvedConflict: ResolveConflict = await this.resolveFileExists(
         { name: conflict.name, isFolder } as Resource,
         conflictsLeft,
         conflictsLeft === 1,
-        isFolder
+        isFolder,
+        true
       )
-      count++
+      isFolder ? folderCount++ : fileCount++
       if (resolvedConflict.doForAllConflicts) {
         if (isFolder) {
           doForAllConflictsFolders = true

packages/web-app-files/src/helpers/resource/conflictHandling/conflictDialog.ts

@@ -1,6 +1,5 @@
 import { join } from 'path'
 import { Resource } from 'web-client'
-import { SpaceResource } from 'web-client/src/helpers'
 import { ResolveConflict, ResolveStrategy } from '.'
 
 interface FileConflict {
@@ -25,26 +24,22 @@ export class ConflictDialog {
 
   async resolveAllConflicts(
     resourcesToMove: Resource[],
-    targetSpace: SpaceResource,
     targetFolder: Resource,
     targetFolderResources: Resource[]
-  ) {
+  ): Promise<FileConflict[]> {
     // Collect all conflicting resources
-    const allConflicts = []
+    const allConflicts: FileConflict[] = []
     for (const resource of resourcesToMove) {
       const targetFilePath = join(targetFolder.path, resource.name)
       const exists = targetFolderResources.some((r) => r.path === targetFilePath)
       if (exists) {
-        allConflicts.push({
-          resource,
-          strategy: null
-        } as FileConflict)
+        allConflicts.push({ resource, strategy: null })
       }
     }
     let count = 0
     let doForAllConflicts = false
     let doForAllConflictsStrategy = null
-    const resolvedConflicts = []
+    const resolvedConflicts: FileConflict[] = []
     for (const conflict of allConflicts) {
       // Resolve conflicts accordingly
       if (doForAllConflicts) {
@@ -74,22 +69,35 @@ export class ConflictDialog {
     return resolvedConflicts
   }
 
-  async resolveFileExists(
+  resolveFileExists(
     resource: Resource,
     conflictCount: number,
     isSingleConflict: boolean,
-    suggestMerge = false
+    suggestMerge = false,
+    separateSkipHandling = false // separate skip-handling between files and folders
   ): Promise<ResolveConflict> {
+    let translatedSkipLabel
+
+    if (!separateSkipHandling) {
+      translatedSkipLabel = this.$gettext('Apply to all %{count} conflicts')
+    } else if (resource.isFolder) {
+      translatedSkipLabel = this.$gettext('Apply to all %{count} folders')
+    } else {
+      translatedSkipLabel = this.$gettext('Apply to all %{count} files')
+    }
+
     return new Promise<ResolveConflict>((resolve) => {
       let doForAllConflicts = false
       const modal = {
         variation: 'danger',
         icon: 'alarm-warning',
-        title: this.$gettext('File already exists'),
+        title: resource.isFolder
+          ? this.$gettext('Folder already exists')
+          : this.$gettext('File already exists'),
         message: this.$gettextInterpolate(
           resource.isFolder
-            ? this.$gettext('Folder with name %{name} already exists.')
-            : this.$gettext('File with name %{name} already exists.'),
+            ? this.$gettext('Folder with name "%{name}" already exists.')
+            : this.$gettext('File with name "%{name}" already exists.'),
           { name: resource.name },
           true
         ),
@@ -98,11 +106,7 @@ export class ConflictDialog {
         buttonSecondaryText: suggestMerge ? this.$gettext('Merge') : this.$gettext('Replace'),
         checkboxLabel: isSingleConflict
           ? ''
-          : this.$gettextInterpolate(
-              this.$gettext('Apply to all %{count} conflicts'),
-              { count: conflictCount },
-              true
-            ),
+          : this.$gettextInterpolate(translatedSkipLabel, { count: conflictCount }, true),
         onCheckboxValueChanged: (value) => {
           doForAllConflicts = value
         },

packages/web-app-files/tests/unit/helpers/resource/actions/upload.spec.ts

@@ -47,6 +47,53 @@ const getResourcesUploadInstance = ({
 }
 
 describe('upload helper', () => {
+  describe('method "perform"', () => {
+    it('should handle the file upload when no conflicts found', () => {
+      const resourcesUpload = getResourcesUploadInstance()
+      const handleFileUppyloadStub = jest.fn()
+      const displayOverwriteDialogStub = jest.fn()
+      resourcesUpload.handleUppyFileUpload = handleFileUppyloadStub
+      resourcesUpload.displayOverwriteDialog = displayOverwriteDialogStub
+      resourcesUpload.perform()
+      expect(handleFileUppyloadStub).toHaveBeenCalledTimes(1)
+      expect(displayOverwriteDialogStub).toHaveBeenCalledTimes(0)
+    })
+    it('should display the overwrite dialog when conflicts found', () => {
+      const resourcesUpload = getResourcesUploadInstance()
+      const handleFileUppyloadStub = jest.fn()
+      const displayOverwriteDialogStub = jest.fn()
+      const getConflictsStub = jest.fn(() => [{ name: 'name', type: 'file' }])
+      resourcesUpload.handleUppyFileUpload = handleFileUppyloadStub
+      resourcesUpload.displayOverwriteDialog = displayOverwriteDialogStub
+      resourcesUpload.getConflicts = getConflictsStub
+      resourcesUpload.perform()
+      expect(handleFileUppyloadStub).toHaveBeenCalledTimes(0)
+      expect(displayOverwriteDialogStub).toHaveBeenCalledTimes(1)
+    })
+  })
+  describe('method "getConflicts"', () => {
+    it('should return file and folder conflicts', () => {
+      const fileName = 'someFile.txt'
+      const folderName = 'someFolder'
+      const currentFiles = [
+        mockDeep<Resource>({ name: fileName }),
+        mockDeep<Resource>({ name: folderName })
+      ]
+      const filesToUpload = [
+        mockDeep<UppyResource>({ name: fileName, meta: { relativePath: '', relativeFolder: '' } }),
+        mockDeep<UppyResource>({
+          name: 'anotherFile',
+          meta: { relativePath: `/${folderName}/anotherFile` }
+        })
+      ]
+      const resourcesUpload = getResourcesUploadInstance({ currentFiles })
+      const conflicts = resourcesUpload.getConflicts(filesToUpload)
+
+      expect(conflicts.length).toBe(2)
+      expect(conflicts).toContainEqual({ name: fileName, type: 'file' })
+      expect(conflicts).toContainEqual({ name: folderName, type: 'folder' })
+    })
+  })
   describe('method "checkQuotaExceeded"', () => {
     const remainingQuota = 1000
     const spacesMock = mockDeep<SpaceResource>({
@@ -158,7 +205,7 @@ describe('upload helper', () => {
     })
   })
 
-  describe('upload conflict dialog', () => {
+  describe('method "displayOverwriteDialog"', () => {
     it.each([ResolveStrategy.REPLACE, ResolveStrategy.KEEP_BOTH])(
       'should upload file if user chooses replace or keep both',
       async (strategy) => {

packages/web-app-files/tests/unit/helpers/resource/conflictHandling/conflictDialog.spec.ts

@@ -0,0 +1,40 @@
+import { mockDeep } from 'jest-mock-extended'
+import { Resource } from 'web-client'
+import { ConflictDialog, ResolveConflict } from 'web-app-files/src/helpers/resource'
+
+const getConflictDialogInstance = ({ createModal = jest.fn() } = {}) => {
+  return new ConflictDialog(createModal, jest.fn(), jest.fn(), jest.fn(), jest.fn(), jest.fn())
+}
+
+describe('conflict dialog', () => {
+  describe('method "resolveAllConflicts"', () => {
+    it('should return the resolved conflicts including the resource(s) and the strategy', async () => {
+      const conflictDialog = getConflictDialogInstance()
+      const strategy = mockDeep<ResolveConflict>()
+      conflictDialog.resolveFileExists = jest.fn().mockImplementation(() => ({
+        strategy,
+        doForAllConflicts: false
+      }))
+      const resource = mockDeep<Resource>({ name: 'someFile.txt' })
+      const targetFolder = mockDeep<Resource>({ path: '/' })
+      const targetFolderResources = [mockDeep<Resource>({ path: '/someFile.txt' })]
+      const resolvedConflicts = await conflictDialog.resolveAllConflicts(
+        [resource],
+        targetFolder,
+        targetFolderResources
+      )
+
+      expect(resolvedConflicts.length).toBe(1)
+      expect(resolvedConflicts[0].resource).toEqual(resource)
+      expect(resolvedConflicts[0].strategy).toEqual(strategy)
+    })
+  })
+  describe('method "resolveFileExists"', () => {
+    it('should create the modal in the end', () => {
+      const createModal = jest.fn()
+      const conflictDialog = getConflictDialogInstance({ createModal })
+      conflictDialog.resolveFileExists(mockDeep<Resource>(), 2, true)
+      expect(createModal).toHaveBeenCalledTimes(1)
+    })
+  })
+})

packages/web-app-files/tests/unit/helpers/resource/resourcesTransfer.spec.ts

@@ -137,12 +137,7 @@ describe('resourcesTransfer', () => {
     resourcesTransfer.resolveFileExists = jest
       .fn()
       .mockImplementation(() => Promise.resolve({ strategy: 0 } as ResolveConflict))
-    await resourcesTransfer.resolveAllConflicts(
-      resourcesToMove,
-      targetSpace,
-      targetFolder,
-      targetFolderItems
-    )
+    await resourcesTransfer.resolveAllConflicts(resourcesToMove, targetFolder, targetFolderItems)
 
     expect(resourcesTransfer.resolveFileExists).toHaveBeenCalled()
   })

tests/acceptance/features/webUIFilesCopy/copy.feature

@@ -32,7 +32,7 @@ Feature: copy files and folders
     And user "Alice" has logged in using the webUI
     And the user has browsed to the personal page
     When the user tries to copy file "strängé filename (duplicate #2 &).txt" into folder "strängé नेपाली folder" using the webUI
-    Then the "modal error" message with header 'File with name strängé filename (duplicate #2 &).txt already exists.' should be displayed on the webUI
+    Then the "modal error" message with header 'File with name "strängé filename (duplicate #2 &).txt" already exists.' should be displayed on the webUI
 
   @smokeTest @ocisSmokeTest
   Scenario: Copy multiple files at once

tests/acceptance/features/webUIMoveFilesFolders/moveFiles.feature

@@ -45,7 +45,7 @@ Feature: move files
     And user "Alice" has uploaded file "lorem.txt" to "simple-folder/lorem.txt" in the server
     And the user has browsed to the personal page
     When the user tries to move file "lorem.txt" into folder "simple-folder" using the webUI
-    Then the "modal error" message with header 'File with name lorem.txt already exists.' should be displayed on the webUI
+    Then the "modal error" message with header 'File with name "lorem.txt" already exists.' should be displayed on the webUI
 
   @smokeTest @ocisSmokeTest  @disablePreviews
   Scenario: Move multiple files at once

tests/acceptance/features/webUIMoveFilesFolders/moveFolders.feature

@@ -35,7 +35,7 @@ Feature: move folders
     And user "Alice" has logged in using the webUI
     And the user has browsed to the personal page
     When the user tries to move folder "simple-empty-folder" into folder "simple-folder" using the webUI
-    Then the "modal error" message with header 'Folder with name simple-empty-folder already exists.' should be displayed on the webUI
+    Then the "modal error" message with header 'Folder with name "simple-empty-folder" already exists.' should be displayed on the webUI
 
   @smokeTest
   Scenario: Move multiple folders at once