From a315450af731f25b4d8b67894680f8b816ea779d Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Wed, 31 Jan 2024 08:49:03 +0100 Subject: [PATCH 1/2] refactor: remove share property on ShareResource Removes the `share` property on `ShareResource` because a) there is no need to expose the full share on a resource and b) a `ShareResource` can "hold" multiple shares, rendering that property useless. --- .../src/components/SideBar/Versions/FileVersions.vue | 5 ----- .../web-app-files/src/views/shares/SharedWithOthers.vue | 6 ++++-- .../unit/components/SideBar/Versions/FileVersions.spec.ts | 7 ++----- .../tests/unit/views/shares/SharedWithOthers.spec.ts | 6 +++--- packages/web-client/src/helpers/share/functions.ts | 1 - packages/web-client/src/helpers/share/functionsNG.ts | 2 -- packages/web-client/src/helpers/share/types.ts | 2 -- 7 files changed, 9 insertions(+), 20 deletions(-) diff --git a/packages/web-app-files/src/components/SideBar/Versions/FileVersions.vue b/packages/web-app-files/src/components/SideBar/Versions/FileVersions.vue index ca8650460ff..84820d78c29 100644 --- a/packages/web-app-files/src/components/SideBar/Versions/FileVersions.vue +++ b/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 diff --git a/packages/web-app-files/src/views/shares/SharedWithOthers.vue b/packages/web-app-files/src/views/shares/SharedWithOthers.vue index 94a2deba7ae..462fe663e87 100644 --- a/packages/web-app-files/src/views/shares/SharedWithOthers.vue +++ b/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)) }) }) diff --git a/packages/web-app-files/tests/unit/components/SideBar/Versions/FileVersions.spec.ts b/packages/web-app-files/tests/unit/components/SideBar/Versions/FileVersions.spec.ts index b1c1d5c5880..725548739c6 100644 --- a/packages/web-app-files/tests/unit/components/SideBar/Versions/FileVersions.spec.ts +++ b/packages/web-app-files/tests/unit/components/SideBar/Versions/FileVersions.spec.ts @@ -111,10 +111,7 @@ describe('FileVersions', () => { expect(revertVersionButton.length).toBe(defaultVersions.length) }) it('should be possible for a share with write permissions', async () => { - const resource = mockDeep({ - permissions: DavPermission.Updateable, - share: undefined - }) + const resource = mockDeep({ permissions: DavPermission.Updateable }) const space = mockDeep({ driveType: 'share' }) const { wrapper } = getMountedWrapper({ resource, space }) await wrapper.vm.fetchVersionsTask.last @@ -122,7 +119,7 @@ describe('FileVersions', () => { expect(revertVersionButton.length).toBe(defaultVersions.length) }) it('should not be possible for a share with read-only permissions', async () => { - const resource = mockDeep({ permissions: '', share: undefined }) + const resource = mockDeep({ permissions: '' }) const space = mockDeep({ driveType: 'share' }) const { wrapper } = getMountedWrapper({ resource, space }) await wrapper.vm.fetchVersionsTask.last diff --git a/packages/web-app-files/tests/unit/views/shares/SharedWithOthers.spec.ts b/packages/web-app-files/tests/unit/views/shares/SharedWithOthers.spec.ts index 5531504caff..b79644ee907 100644 --- a/packages/web-app-files/tests/unit/views/shares/SharedWithOthers.spec.ts +++ b/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({ share: { shareType: ShareTypes.user.value } }), - mock({ share: { shareType: ShareTypes.group.value } }) + mock({ shareTypes: [ShareTypes.user.value] }), + mock({ 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({ share: { shareType: ShareTypes.user.value } })] + files: [mock({ shareTypes: [ShareTypes.user.value] })] }) expect(wrapper.find('.share-type-filter').exists()).toBeFalsy() }) diff --git a/packages/web-client/src/helpers/share/functions.ts b/packages/web-client/src/helpers/share/functions.ts index 3fd6fbfdf08..fc46a498e2f 100644 --- a/packages/web-client/src/helpers/share/functions.ts +++ b/packages/web-client/src/helpers/share/functions.ts @@ -196,7 +196,6 @@ export function buildSharedResource( 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) diff --git a/packages/web-client/src/helpers/share/functionsNG.ts b/packages/web-client/src/helpers/share/functionsNG.ts index 09821eae501..23a2cef1788 100644 --- a/packages/web-client/src/helpers/share/functionsNG.ts +++ b/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', diff --git a/packages/web-client/src/helpers/share/types.ts b/packages/web-client/src/helpers/share/types.ts index 6330404ee77..4280e3e03b0 100644 --- a/packages/web-client/src/helpers/share/types.ts +++ b/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 {} From 28f4542255f2a1d60076151ac7c0aa03e7516a0f Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Wed, 31 Jan 2024 10:36:34 +0100 Subject: [PATCH 2/2] refactor: remove aggregateResourceShares --- .../src/services/folder/loaderSpace.ts | 23 +-- .../web-client/src/helpers/share/functions.ts | 190 +----------------- .../objects/app-files/resource/actions.ts | 5 +- 3 files changed, 15 insertions(+), 203 deletions(-) diff --git a/packages/web-app-files/src/services/folder/loaderSpace.ts b/packages/web-app-files/src/services/folder/loaderSpace.ts index acf2b8bf9d6..33398166a0f 100644 --- a/packages/web-app-files/src/services/folder/loaderSpace.ts +++ b/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 diff --git a/packages/web-client/src/helpers/share/functions.ts b/packages/web-client/src/helpers/share/functions.ts index fc46a498e2f..92ec57c19e1 100644 --- a/packages/web-client/src/helpers/share/functions.ts +++ b/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,186 +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//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.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) diff --git a/tests/e2e/support/objects/app-files/resource/actions.ts b/tests/e2e/support/objects/app-files/resource/actions.ts index 2cf1e36c0c1..671d6e40121 100644 --- a/tests/e2e/support/objects/app-files/resource/actions.ts +++ b/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() ])