fix: trash bin breaking on parent folder navigation

Fixes an issue where the trash bin would break when navigating via the parent folder of a resource. Also adds an interface for trashed resources to better distinguish them from regular resources.
owncloud · Jul 2, 2024 · a1c809c · a1c809c
1 parent c905da6
commit a1c809c
Show file tree

Hide file tree

Showing 9 changed files with 49 additions and 34 deletions.
diff --git a/changelog/unreleased/bugfix-trash-bin-break-on-navigation b/changelog/unreleased/bugfix-trash-bin-break-on-navigation
@@ -0,0 +1,7 @@
+Bugfix: Trash bin breaking on navigation
+
+We've fixed a bug where the trash bin would break when navigating into the parent folder of a resource.
+
+https://github.com/owncloud/web/pull/11132
+https://github.com/owncloud/web/issues/11100
+https://github.com/owncloud/web/issues/10686
diff --git a/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue b/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue
@@ -140,7 +140,7 @@ import {
   formatDateFromJSDate
 } from '@ownclouders/web-pkg'
 import upperFirst from 'lodash-es/upperFirst'
-import { isShareResource, ShareTypes } from '@ownclouders/web-client'
+import { isShareResource, isTrashResource, ShareTypes } from '@ownclouders/web-client'
 import { usePreviewService, useGetMatchingSpace } from '@ownclouders/web-pkg'
 import { getIndicators } from '@ownclouders/web-pkg'
 import {
@@ -256,11 +256,15 @@ export default defineComponent({
     })
 
     const hasDeletionDate = computed(() => {
-      return unref(resource).ddate
+      return isTrashResource(unref(resource))
     })
 
     const capitalizedDeletionDate = computed(() => {
-      const displayDate = formatDateFromJSDate(new Date(unref(resource).ddate), language.current)
+      const item = unref(resource)
+      if (!isTrashResource(item)) {
+        return ''
+      }
+      const displayDate = formatDateFromJSDate(new Date(item.ddate), language.current)
       return upperFirst(displayDate)
     })
 

diff --git a/packages/web-client/src/helpers/resource/functions.ts b/packages/web-client/src/helpers/resource/functions.ts
@@ -1,13 +1,17 @@
 import path, { basename, dirname } from 'path'
 import { urlJoin } from '../../utils'
 import { DavPermission, DavProperty } from '../../webdav/constants'
-import { Resource, ResourceIndicator, WebDavResponseResource } from './types'
+import { Resource, ResourceIndicator, TrashResource, WebDavResponseResource } from './types'
 import { camelCase } from 'lodash-es'
 
 const fileExtensions = {
   complex: ['tar.bz2', 'tar.gz', 'tar.xz']
 }
 
+export const isTrashResource = (resource: Resource): resource is TrashResource => {
+  return Object.hasOwn(resource, 'ddate')
+}
+
 export const extractDomSelector = (str: string): string => {
   return str.replace(/[^A-Za-z0-9\-_]/g, '')
 }
@@ -202,7 +206,7 @@ export function buildResource(resource: WebDavResponseResource): Resource {
   return r
 }
 
-export function buildDeletedResource(resource: WebDavResponseResource): Resource {
+export function buildDeletedResource(resource: WebDavResponseResource): TrashResource {
   const isFolder = resource.type === 'directory'
   const fullName = resource.props[DavProperty.TrashbinOriginalFilename]?.toString()
   const extension = extractExtensionFromFile({ name: fullName, type: resource.type } as Resource)

diff --git a/packages/web-client/src/helpers/resource/types.ts b/packages/web-client/src/helpers/resource/types.ts
@@ -78,7 +78,6 @@ export interface Resource {
   privateLink?: string
   owner?: Identity
   extension?: string
-  ddate?: string
 
   // necessary for incoming share resources and resources inside shares
   remoteItemId?: string
@@ -90,7 +89,6 @@ export interface Resource {
   canShare?(args?: { user?: User; ability?: Ability }): boolean
   canRename?(args?: { user?: User; ability?: Ability }): boolean
   canBeDeleted?(args?: { user?: User; ability?: Ability }): boolean
-  canBeRestored?(): boolean
   canDeny?(): boolean
   canRemoveFromTrashbin?({ user }: { user?: User }): boolean
   canEditTags?(): boolean
@@ -118,6 +116,11 @@ export interface FileResource extends Resource {
   __fileResource?: any
 }
 
+export interface TrashResource extends Resource {
+  ddate?: string
+  canBeRestored?(): boolean
+}
+
 export interface WebDavResponseTusSupport {
   extension?: string[]
   maxSize?: number

diff --git a/packages/web-pkg/src/components/FilesList/ResourceTable.vue b/packages/web-pkg/src/components/FilesList/ResourceTable.vue
@@ -243,7 +243,7 @@ import {
   ComponentPublicInstance
 } from 'vue'
 import { useWindowSize } from '@vueuse/core'
-import { IncomingShareResource, Resource } from '@ownclouders/web-client'
+import { IncomingShareResource, Resource, TrashResource } from '@ownclouders/web-client'
 import { extractDomSelector, SpaceResource } from '@ownclouders/web-client'
 import { ShareTypes, isShareResource } from '@ownclouders/web-client'
 
@@ -756,9 +756,9 @@ export default defineComponent({
               wrap: 'nowrap',
               width: 'shrink',
               accessibleLabelCallback: (item) =>
-                this.formatDateRelative((item as Resource).ddate) +
+                this.formatDateRelative((item as TrashResource).ddate) +
                 ' (' +
-                this.formatDate((item as Resource).ddate) +
+                this.formatDate((item as TrashResource).ddate) +
                 ')'
             }
           ] as FieldType[]

diff --git a/packages/web-pkg/src/composables/actions/files/useFileActionsRestore.ts b/packages/web-pkg/src/composables/actions/files/useFileActionsRestore.ts
@@ -5,7 +5,8 @@ import {
   Resource,
   isProjectSpaceResource,
   extractExtensionFromFile,
-  SpaceResource
+  SpaceResource,
+  isTrashResource
 } from '@ownclouders/web-client'
 import {
   ResolveStrategy,
@@ -213,7 +214,7 @@ export const useFileActionsRestore = () => {
         if (!isLocationTrashActive(router, 'files-trash-generic')) {
           return false
         }
-        if (!resources.every((r) => r.canBeRestored())) {
+        if (!resources.every((r) => isTrashResource(r) && r.canBeRestored())) {
           return false
         }
 

diff --git a/packages/web-pkg/tests/unit/components/FilesList/ResourceTable.spec.ts b/packages/web-pkg/tests/unit/components/FilesList/ResourceTable.spec.ts
@@ -136,7 +136,6 @@ const resourcesWithAllFields = [
     tags: ['space', 'tag'],
     mdate: getCurrentDate(),
     sdate: getCurrentDate(),
-    ddate: getCurrentDate(),
     owner,
     sharedBy,
     sharedWith,
@@ -161,7 +160,6 @@ const resourcesWithAllFields = [
     tags: [],
     mdate: getCurrentDate(),
     sdate: getCurrentDate(),
-    ddate: getCurrentDate(),
     owner,
     sharedBy,
     sharedWith,
@@ -185,7 +183,6 @@ const resourcesWithAllFields = [
     size: '237895',
     mdate: getCurrentDate(),
     sdate: getCurrentDate(),
-    ddate: getCurrentDate(),
     owner,
     sharedBy,
     sharedWith,
@@ -243,7 +240,6 @@ const processingResourcesWithAllFields = [
     tags: ['space', 'tag'],
     mdate: getCurrentDate(),
     sdate: getCurrentDate(),
-    ddate: getCurrentDate(),
     owner,
     sharedBy,
     sharedWith,

diff --git a/packages/web-pkg/tests/unit/composables/actions/files/useFileActionsEmptyTrashBin.spec.ts b/packages/web-pkg/tests/unit/composables/actions/files/useFileActionsEmptyTrashBin.spec.ts
@@ -3,7 +3,7 @@ import { useMessages, useModals } from '../../../../../src/composables/piniaStor
 import { mock } from 'vitest-mock-extended'
 import { getComposableWrapper, defaultComponentMocks, RouteLocation } from 'web-test-helpers'
 import { unref } from 'vue'
-import { ProjectSpaceResource, Resource } from '@ownclouders/web-client'
+import { ProjectSpaceResource, TrashResource } from '@ownclouders/web-client'
 import { FileActionOptions } from '../../../../../src/composables/actions'
 
 describe('emptyTrashBin', () => {
@@ -23,7 +23,7 @@ describe('emptyTrashBin', () => {
           expect(
             unref(actions)[0].isVisible({
               space,
-              resources: [{ canBeRestored: () => true }] as Resource[]
+              resources: [{ canBeRestored: () => true }] as TrashResource[]
             })
           ).toBe(false)
         }

diff --git a/packages/web-pkg/tests/unit/composables/actions/files/useFileActionsRestore.spec.ts b/packages/web-pkg/tests/unit/composables/actions/files/useFileActionsRestore.spec.ts
@@ -3,7 +3,7 @@ import { mock } from 'vitest-mock-extended'
 import { defaultComponentMocks, getComposableWrapper, RouteLocation } from 'web-test-helpers'
 import { useMessages, useResourcesStore } from '../../../../../src/composables/piniaStores'
 import { unref } from 'vue'
-import { HttpError, Resource } from '@ownclouders/web-client'
+import { HttpError, Resource, TrashResource } from '@ownclouders/web-client'
 import { ProjectSpaceResource, SpaceResource } from '@ownclouders/web-client'
 import { Drive } from '@ownclouders/web-client/graph/generated'
 import { AxiosResponse } from 'axios'
@@ -16,12 +16,7 @@ describe('restore', () => {
     it('should be false when no resource is given', () => {
       getWrapper({
         setup: ({ actions }, { space }) => {
-          expect(
-            unref(actions)[0].isVisible({
-              space,
-              resources: [] as Resource[]
-            })
-          ).toBe(false)
+          expect(unref(actions)[0].isVisible({ space, resources: [] })).toBe(false)
         }
       })
     })
@@ -31,7 +26,7 @@ describe('restore', () => {
           expect(
             unref(actions)[0].isVisible({
               space,
-              resources: [{ canBeRestored: () => true }] as Resource[]
+              resources: [{ canBeRestored: () => true, ddate: '2020-01-01' }] as TrashResource[]
             })
           ).toBe(true)
         }
@@ -43,7 +38,7 @@ describe('restore', () => {
           expect(
             unref(actions)[0].isVisible({
               space,
-              resources: [{ canBeRestored: () => false }] as Resource[]
+              resources: [{ canBeRestored: () => false }] as TrashResource[]
             })
           ).toBe(false)
         }
@@ -53,7 +48,9 @@ describe('restore', () => {
       getWrapper({
         invalidLocation: true,
         setup: ({ actions }, { space }) => {
-          expect(unref(actions)[0].isVisible({ space, resources: [{}] as Resource[] })).toBe(false)
+          expect(unref(actions)[0].isVisible({ space, resources: [{}] as TrashResource[] })).toBe(
+            false
+          )
         }
       })
     })
@@ -64,7 +61,7 @@ describe('restore', () => {
           expect(
             unref(actions)[0].isVisible({
               space,
-              resources: [{ canBeRestored: () => true }] as Resource[]
+              resources: [{ canBeRestored: () => true }] as TrashResource[]
             })
           ).toBe(false)
         }
@@ -116,7 +113,7 @@ describe('restore', () => {
   it('should request parent folder on collecting restore conflicts', () => {
     getWrapper({
       setup: async ({ collectConflicts }, { space, clientService }) => {
-        const resource = { id: '1', path: '1', name: '1' } as Resource
+        const resource = { id: '1', path: '1', name: '1' } as TrashResource
         await collectConflicts(space, [resource])
 
         expect(clientService.webdav.listFiles).toHaveBeenCalledWith(expect.anything(), {
@@ -129,8 +126,8 @@ describe('restore', () => {
   it('should find conflict within resources', () => {
     getWrapper({
       setup: async ({ collectConflicts }, { space }) => {
-        const resourceOne = { id: '1', path: '1', name: '1' } as Resource
-        const resourceTwo = { id: '2', path: '1', name: '1' } as Resource
+        const resourceOne = { id: '1', path: '1', name: '1' } as TrashResource
+        const resourceTwo = { id: '2', path: '1', name: '1' } as TrashResource
         const { conflicts } = await collectConflicts(space, [resourceOne, resourceTwo])
 
         expect(conflicts).toContain(resourceTwo)
@@ -141,7 +138,7 @@ describe('restore', () => {
   it('should add files without conflict to resolved resources', () => {
     getWrapper({
       setup: async ({ collectConflicts }, { space }) => {
-        const resource = { id: '1', path: '1', name: '1' } as Resource
+        const resource = { id: '1', path: '1', name: '1' } as TrashResource
         const { resolvedResources } = await collectConflicts(space, [resource])
 
         expect(resolvedResources).toContain(resource)
@@ -158,7 +155,10 @@ function getWrapper({
 }: {
   invalidLocation?: boolean
   driveType?: string
-  restoreResult?: { successful: Resource[]; failed: { resource: Resource; error: HttpError }[] }
+  restoreResult?: {
+    successful: TrashResource[]
+    failed: { resource: TrashResource; error: HttpError }[]
+  }
   setup: (
     instance: ReturnType<typeof useFileActionsRestore>,
     {