Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from
Aug 3, 2023
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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)
}
}

23 changes: 18 additions & 5 deletions packages/web-app-files/src/store/actions.ts
Original file line number Diff line number Diff line change
@@ -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) {
Original file line number Diff line number Diff line change
@@ -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: [] } }
Original file line number Diff line number Diff line change
@@ -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)
}
})
})
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -67,7 +67,6 @@ module.exports = {
*/
delete: async function () {
await this.performFileAction(this.FileAction.delete)
await this.api.page.FilesPageElement.filesList().confirmDeletion()
return this
},
/**
4 changes: 0 additions & 4 deletions tests/acceptance/pageObjects/personalPage.js
Original file line number Diff line number Diff line change
@@ -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
Loading