Merge pull request #10426 from owncloud/remove-share-prop-on-share-re…

…sources

refactor: remove aggregateResourceShares and share prop on share resources

packages/web-app-files/src/components/SideBar/Versions/FileVersions.vue

@@ -65,7 +65,6 @@ import {
 } from '@ownclouders/web-pkg'
 import { computed, defineComponent, inject, Ref, ref, unref, watch } from 'vue'
 import { isShareSpaceResource, Resource, SpaceResource } from '@ownclouders/web-client/src/helpers'
-import { SharePermissions, isShareResource } from '@ownclouders/web-client/src/helpers/share'
 import { useTask } from 'vue-concurrency'
 import { useGettext } from 'vue3-gettext'
 
@@ -120,10 +119,6 @@ export default defineComponent({
         if (unref(resource).permissions !== undefined) {
           return unref(resource).permissions.includes(DavPermission.Updateable)
         }
-        const res = unref(resource)
-        if (isShareResource(res) && res.share?.role) {
-          return res.share.role.hasPermission(SharePermissions.update)
-        }
       }
 
       return true

packages/web-app-files/src/services/folder/loaderSpace.ts

@@ -12,7 +12,7 @@ import { unref } from 'vue'
 import { FolderLoaderOptions } from './types'
 import { authService } from 'web-runtime/src/services/auth'
 import { useFileRouteReplace } from '@ownclouders/web-pkg'
-import { aggregateResourceShares } from '@ownclouders/web-client/src/helpers/share'
+import { IncomingShareResource } from '@ownclouders/web-client/src/helpers/share'
 import { getIndicators } from '@ownclouders/web-pkg'
 
 export class FolderLoaderSpace implements FolderLoader {
@@ -32,9 +32,8 @@ export class FolderLoaderSpace implements FolderLoader {
   }
 
   public getTask(context: TaskContext): FolderLoaderTask {
-    const { spacesStore, router, clientService, configStore, capabilityStore, resourcesStore } =
-      context
-    const { owncloudSdk: client, webdav } = clientService
+    const { router, clientService, resourcesStore } = context
+    const { webdav } = clientService
     const { replaceInvalidFileRoute } = useFileRouteReplace({ router })
 
     return useTask(function* (
@@ -60,15 +59,13 @@ export class FolderLoaderSpace implements FolderLoader {
 
         if (path === '/') {
           if (isShareSpaceResource(space)) {
-            const parentShare = yield client.shares.getShare(space.shareId)
-            const aggregatedShares = aggregateResourceShares({
-              shares: [parentShare.shareInfo],
-              spaces: spacesStore.spaces,
-              allowSharePermission: capabilityStore.sharingResharing,
-              incomingShares: true,
-              fullShareOwnerPaths: configStore.options.routing.fullShareOwnerPaths
-            })
-            currentFolder = aggregatedShares[0]
+            // FIXME: it would be cleaner to fetch the driveItem as soon as graph api is capable of it
+            currentFolder = {
+              ...currentFolder,
+              id: space.shareId,
+              syncEnabled: true,
+              canShare: () => false
+            } as IncomingShareResource
           } else if (!isPersonalSpaceResource(space) && !isPublicSpaceResource(space)) {
             // note: in the future we might want to show the space as root for personal spaces as well (to show quota and the like). Currently not needed.
             currentFolder = space

packages/web-app-files/src/views/shares/SharedWithOthers.vue

@@ -145,7 +145,7 @@ export default defineComponent({
       resourcesViewDefaults
 
     const shareTypes = computed(() => {
-      const uniqueShareTypes = uniq(unref(paginatedResources).map((i) => i.share?.shareType))
+      const uniqueShareTypes = uniq(unref(paginatedResources).flatMap((i) => i.shareTypes))
       return ShareTypes.getByValues(uniqueShareTypes)
     })
     const selectedShareTypesQuery = useRouteQuery('q_shareType')
@@ -155,7 +155,9 @@ export default defineComponent({
         return unref(paginatedResources)
       }
       return unref(paginatedResources).filter((item) => {
-        return selectedShareTypes.map((t) => ShareTypes[t].value).includes(item.share.shareType)
+        return selectedShareTypes
+          .map((t) => ShareTypes[t].value)
+          .some((t) => item.shareTypes.includes(t))
       })
     })
 

packages/web-app-files/tests/unit/components/SideBar/Versions/FileVersions.spec.ts

@@ -111,18 +111,15 @@ describe('FileVersions', () => {
               expect(revertVersionButton.length).toBe(defaultVersions.length)
             })
             it('should be possible for a share with write permissions', async () => {
-              const resource = mockDeep<ShareResource>({
-                permissions: DavPermission.Updateable,
-                share: undefined
-              })
+              const resource = mockDeep<ShareResource>({ permissions: DavPermission.Updateable })
               const space = mockDeep<ShareSpaceResource>({ driveType: 'share' })
               const { wrapper } = getMountedWrapper({ resource, space })
               await wrapper.vm.fetchVersionsTask.last
               const revertVersionButton = wrapper.findAll(selectors.revertVersionButton)
               expect(revertVersionButton.length).toBe(defaultVersions.length)
             })
             it('should not be possible for a share with read-only permissions', async () => {
-              const resource = mockDeep<ShareResource>({ permissions: '', share: undefined })
+              const resource = mockDeep<ShareResource>({ permissions: '' })
               const space = mockDeep<ShareSpaceResource>({ driveType: 'share' })
               const { wrapper } = getMountedWrapper({ resource, space })
               await wrapper.vm.fetchVersionsTask.last

packages/web-app-files/tests/unit/views/shares/SharedWithOthers.spec.ts

@@ -52,15 +52,15 @@ describe('SharedWithOthers view', () => {
       it('shows filter if multiple share types are present', () => {
         const { wrapper } = getMountedWrapper({
           files: [
-            mock<ShareResource>({ share: { shareType: ShareTypes.user.value } }),
-            mock<ShareResource>({ share: { shareType: ShareTypes.group.value } })
+            mock<ShareResource>({ shareTypes: [ShareTypes.user.value] }),
+            mock<ShareResource>({ shareTypes: [ShareTypes.group.value] })
           ]
         })
         expect(wrapper.find('.share-type-filter').exists()).toBeTruthy()
       })
       it('does not show filter if only one share type is present', () => {
         const { wrapper } = getMountedWrapper({
-          files: [mock<ShareResource>({ share: { shareType: ShareTypes.user.value } })]
+          files: [mock<ShareResource>({ shareTypes: [ShareTypes.user.value] })]
         })
         expect(wrapper.find('.share-type-filter').exists()).toBeFalsy()
       })

packages/web-client/src/helpers/share/functions.ts

@@ -1,14 +1,6 @@
-import { orderBy } from 'lodash-es'
 import { DateTime } from 'luxon'
-import {
-  Resource,
-  extractDomSelector,
-  extractExtensionFromFile,
-  extractStorageId
-} from '../resource'
+import { Resource } from '../resource'
 import { ShareTypes } from './type'
-import path from 'path'
-import { SHARE_JAIL_ID, SpaceResource, buildWebDavSpacesPath } from '../space'
 import { SharePermissions } from './permission'
 import { buildSpaceShare } from './space'
 import { LinkShareRoles, PeopleShareRoles } from './role'
@@ -26,187 +18,6 @@ export const isIncomingShareResource = (resource: Resource): resource is Incomin
   return isShareResource(resource) && !resource.outgoing
 }
 
-/**
- * Transforms given shares into a resource format and returns only their unique occurences
- *
- * @deprecated
- */
-export function aggregateResourceShares({
-  shares,
-  spaces,
-  allowSharePermission = true,
-  incomingShares = false,
-  fullShareOwnerPaths = false
-}: {
-  shares: Share[]
-  spaces: SpaceResource[]
-  allowSharePermission?: boolean
-  incomingShares?: boolean
-  fullShareOwnerPaths?: boolean
-}): Resource[] {
-  shares.sort((a, b) => a.path.localeCompare(b.path))
-  if (spaces.length) {
-    shares = addMatchingSpaceToShares(shares, spaces)
-  }
-  if (incomingShares) {
-    shares = addSharedWithToShares(shares)
-    return orderBy(shares, ['file_source', 'permissions'], ['asc', 'desc']).map((share) => {
-      const resource = buildSharedResource(share, incomingShares, allowSharePermission)
-      resource.shareId = share.id
-
-      if (fullShareOwnerPaths) {
-        resource.path = spaces.find(
-          (space) =>
-            space.driveType === 'mountpoint' &&
-            space.id === `${SHARE_JAIL_ID}$${SHARE_JAIL_ID}!${resource.shareId}`
-        )?.root.remoteItem.path
-        resource.shareRoot = resource.path
-      }
-      return resource
-    })
-  }
-
-  const resources = addSharedWithToShares(shares)
-  return resources.map((share) => buildSharedResource(share, incomingShares, allowSharePermission))
-}
-
-function addSharedWithToShares(shares) {
-  const resources = []
-  let previousShare = null
-  for (const share of shares) {
-    if (
-      previousShare?.storage_id === share.storage_id &&
-      previousShare?.file_source === share.file_source
-    ) {
-      if (ShareTypes.containsAnyValue(ShareTypes.authenticated, [parseInt(share.share_type)])) {
-        if (share.stime > previousShare.stime) {
-          previousShare.stime = share.stime
-        }
-        previousShare.sharedWith.push({
-          id: share.share_with,
-          displayName: share.share_with_displayname
-        })
-      } else if (parseInt(share.share_type) === ShareTypes.link.value) {
-        previousShare.sharedWith.push({
-          id: share.token,
-          displayName: share.name || share.token
-        })
-      }
-
-      continue
-    }
-
-    if (ShareTypes.containsAnyValue(ShareTypes.authenticated, [parseInt(share.share_type)])) {
-      share.sharedWith = [
-        {
-          id: share.share_with,
-          displayName: share.share_with_displayname
-        }
-      ]
-    } else if (parseInt(share.share_type) === ShareTypes.link.value) {
-      share.sharedWith = [
-        {
-          id: share.token,
-          displayName: share.name || share.token
-        }
-      ]
-    }
-
-    previousShare = share
-    resources.push(share)
-  }
-  return resources
-}
-
-function addMatchingSpaceToShares(shares, spaces) {
-  const resources = []
-  for (const share of shares) {
-    let matchingSpace
-    if (share.path === '/') {
-      const storageId = extractStorageId(share.item_source)
-      matchingSpace = spaces.find((s) => s.id === storageId && s.driveType === 'project')
-    }
-    resources.push({ ...share, matchingSpace })
-  }
-  return resources
-}
-
-/** @deprecated */
-export function buildSharedResource(
-  share,
-  incomingShares = false,
-  allowSharePermission = true
-): ShareResource {
-  const isFolder = share.item_type === 'folder'
-  const isRemoteShare = parseInt(share.share_type) === ShareTypes.remote.value
-  let resource: ShareResource = {
-    id: share.id,
-    fileId: share.item_source,
-    storageId: extractStorageId(share.item_source),
-    parentFolderId: share.file_parent,
-    type: share.item_type,
-    mimeType: share.mimetype,
-    isFolder,
-    sdate: DateTime.fromSeconds(parseInt(share.stime)).toRFC2822(),
-    indicators: [],
-    tags: [],
-    path: undefined,
-    webDavPath: undefined,
-    processing: share.processing || false,
-    shareTypes: [parseInt(share.share_type)],
-    owner: { id: share.uid_owner, displayName: share.displayname_owner },
-    sharedBy: { id: share.uid_owner, displayName: share.displayname_owner },
-    sharedWith: share.sharedWith || [],
-    outgoing: !incomingShares
-  }
-
-  if (incomingShares) {
-    ;(resource as IncomingShareResource).syncEnabled = parseInt(share.state) === 0
-    ;(resource as IncomingShareResource).hidden = share.hidden === 'true' || share.hidden === true
-    resource.name = isRemoteShare ? share.name : path.basename(share.file_target)
-    if (isRemoteShare) {
-      resource.path = '/'
-      resource.webDavPath = buildWebDavSpacesPath(share.space_id, '/')
-    } else {
-      // FIXME, HACK 1: path needs to be '/' because the share has it's own webdav endpoint (we access it's root). should ideally be removed backend side.
-      // FIXME, HACK 2: webDavPath points to `files/<user>/Shares/xyz` but now needs to point to a shares webdav root. should ideally be changed backend side.
-      resource.path = '/'
-      resource.webDavPath = buildWebDavSpacesPath([SHARE_JAIL_ID, resource.id].join('!'), '/')
-    }
-    resource.canDownload = () => parseInt(share.state) === 0
-    resource.canShare = () => SharePermissions.share.enabled(share.permissions)
-    resource.canRename = () => parseInt(share.state) === 0
-    resource.canBeDeleted = () => SharePermissions.delete.enabled(share.permissions)
-    resource.canEditTags = () =>
-      parseInt(share.state) === 0 && SharePermissions.update.enabled(share.permissions)
-  } else {
-    resource.name = isRemoteShare ? share.name : path.basename(share.path)
-    resource.path = share.path
-    resource.webDavPath = buildWebDavSpacesPath(resource.storageId, share.path)
-
-    resource.canDownload = () => true
-    resource.canShare = () => true
-    resource.canRename = () => true
-    resource.canBeDeleted = () => true
-    resource.canEditTags = () => true
-  }
-
-  resource.extension = extractExtensionFromFile(resource)
-  resource.isReceivedShare = () => incomingShares
-  resource.canUpload = () => SharePermissions.create.enabled(share.permissions)
-  resource.canCreate = () => SharePermissions.create.enabled(share.permissions)
-  resource.isMounted = () => false
-  resource.share = buildShare(share, resource, allowSharePermission)
-  resource.canDeny = () => SharePermissions.denied.enabled(share.permissions)
-  resource.getDomSelector = () => extractDomSelector(share.id)
-
-  if (share.matchingSpace) {
-    resource = { ...resource, ...share.matchingSpace }
-  }
-
-  return resource
-}
-
 export function buildShare(s, file, allowSharePermission): Share {
   if (parseInt(s.share_type) === ShareTypes.link.value) {
     return _buildLink(s)

packages/web-client/src/helpers/share/functionsNG.ts

@@ -93,7 +93,6 @@ export function buildIncomingShareResource({
         : { ...permission?.grantedToV2.user, shareType }
     ],
     shareTypes: [shareType],
-    share: driveItem,
     isFolder: !!driveItem.remoteItem.folder,
     type: !!driveItem.remoteItem.folder ? 'folder' : 'file',
     mimeType: driveItem.remoteItem.file?.mimeType || 'httpd/unix-directory',
@@ -166,7 +165,6 @@ export function buildOutgoingShareResource({
       }
       return ShareTypes.user.value
     }),
-    share: driveItem,
     isFolder: !!driveItem.folder,
     type: !!driveItem.folder ? 'folder' : 'file',
     mimeType: driveItem.file?.mimeType || 'httpd/unix-directory',

packages/web-client/src/helpers/share/types.ts

@@ -23,8 +23,6 @@ export interface ShareResource extends Resource {
   sharedWith: Array<{ shareType: number } & Identity>
   sharedBy: Identity
   outgoing: boolean
-
-  share?: any // FIXME: type DriveItem OR remove? do we want to expose the share on each resource?
 }
 export interface OutgoingShareResource extends ShareResource {}
 

tests/e2e/support/objects/app-files/resource/actions.ts

@@ -124,7 +124,10 @@ export const clickResource = async ({
     const itemId = await resource.locator(fileRow).getAttribute('data-item-id')
     await Promise.all([
       page.waitForResponse(
-        (resp) => resp.url().endsWith(encodeURIComponent(name)) || resp.url().endsWith(itemId)
+        (resp) =>
+          resp.url().endsWith(encodeURIComponent(name)) ||
+          resp.url().endsWith(itemId) ||
+          resp.url().endsWith(encodeURIComponent(itemId))
       ),
       resource.click()
     ])