From d3965230030dfa58a699a6a051088bcf05890912 Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Fri, 2 Sep 2022 12:44:36 +0200 Subject: [PATCH 01/17] Fix sharesTree loading --- .../SideBar/Shares/Collaborators/RoleDropdown.vue | 2 +- .../src/components/SideBar/Shares/FileLinks.vue | 2 +- packages/web-app-files/src/helpers/path.js | 6 +++++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue index 1430e02233e..26b26097532 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue @@ -183,7 +183,7 @@ export default defineComponent({ }, share() { // the root share has an empty key in the shares tree. That's the reason why we retrieve the share by an empty key here - return this.sharesTree['']?.find((s) => s.incoming) + return this.sharesTree['/']?.find((s) => s.incoming) }, allowCustomSharing() { return this.capabilities?.files_sharing?.allow_custom diff --git a/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue b/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue index e527a9967c7..dd2ed57ef7e 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue @@ -184,7 +184,7 @@ export default defineComponent({ share() { // the root share has an empty key in the shares tree. That's the reason why we retrieve the share by an empty key here - return this.sharesTree['']?.find((s) => s.incoming) + return this.sharesTree['/']?.find((s) => s.incoming) }, expirationDate() { diff --git a/packages/web-app-files/src/helpers/path.js b/packages/web-app-files/src/helpers/path.js index cf91b694633..a2a186f0699 100644 --- a/packages/web-app-files/src/helpers/path.js +++ b/packages/web-app-files/src/helpers/path.js @@ -21,12 +21,16 @@ export function getParentPaths(path = '', includeCurrent = false) { const paths = [] const sections = s.split('/') - if (includeCurrent) { + if (includeCurrent && s) { paths.push(s) } sections.pop() while (sections.length > 0) { + if (!sections.join('/')) { + sections.pop() + continue + } paths.push(sections.join('/')) sections.pop() } From edffcd0c7546c7fedddd1d3d6fcb13a27a8659db Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Tue, 6 Sep 2022 13:13:25 +0200 Subject: [PATCH 02/17] Fix parent share fetching in sidebar --- .../Shares/Collaborators/RoleDropdown.vue | 19 ++++---- .../components/SideBar/Shares/FileLinks.vue | 14 +++--- .../src/composables/parentShare/index.ts | 1 + .../parentShare/useIncomingParentShare.ts | 47 +++++++++++++++++++ .../web-app-files/src/helpers/resources.ts | 1 + .../Shares/Collaborators/RoleDropdown.spec.js | 8 +++- .../SideBar/Shares/FileLinks.spec.js | 8 ++++ .../SideBar/Shares/FileShares.spec.js | 4 ++ .../tests/unit/helpers/path.spec.js | 6 +-- 9 files changed, 88 insertions(+), 20 deletions(-) create mode 100644 packages/web-app-files/src/composables/parentShare/index.ts create mode 100644 packages/web-app-files/src/composables/parentShare/useIncomingParentShare.ts diff --git a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue index 26b26097532..c3aac0f1a23 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue @@ -118,6 +118,7 @@ import { import * as uuid from 'uuid' import { defineComponent } from '@vue/runtime-core' import { PropType } from '@vue/composition-api' +import { useIncomingParentShare } from '../../../../composables/parentShare' export default defineComponent({ name: 'RoleDropdown', @@ -147,6 +148,9 @@ export default defineComponent({ required: true } }, + setup() { + return { ...useIncomingParentShare() } + }, data() { return { selectedRole: null, @@ -181,10 +185,6 @@ export default defineComponent({ resourceIsSharable() { return this.allowSharePermission && this.resource.canShare() }, - share() { - // the root share has an empty key in the shares tree. That's the reason why we retrieve the share by an empty key here - return this.sharesTree['/']?.find((s) => s.incoming) - }, allowCustomSharing() { return this.capabilities?.files_sharing?.allow_custom }, @@ -193,9 +193,9 @@ export default defineComponent({ return SpacePeopleShareRoles.list() } - if (this.share?.incoming && this.resourceIsSharable) { + if (this.incomingParentShare && this.resourceIsSharable) { return PeopleShareRoles.filterByBitmask( - parseInt(this.share.permissions), + parseInt(this.incomingParentShare.permissions), this.resource.isFolder, this.allowSharePermission, this.allowCustomSharing !== false @@ -205,8 +205,8 @@ export default defineComponent({ return PeopleShareRoles.list(this.resource.isFolder, this.allowCustomSharing !== false) }, availablePermissions() { - if (this.share?.incoming && this.resourceIsSharable) { - return SharePermissions.bitmaskToPermissions(parseInt(this.share.permissions)) + if (this.incomingParentShare && this.resourceIsSharable) { + return SharePermissions.bitmaskToPermissions(parseInt(this.incomingParentShare.permissions)) } return this.customPermissionsRole.permissions(this.allowSharePermission) }, @@ -223,7 +223,8 @@ export default defineComponent({ window.removeEventListener('keydown', this.cycleRoles) }, - mounted() { + async mounted() { + await this.loadIncomingParentShare.perform(this.resource) this.applyRoleAndPermissions() window.addEventListener('keydown', this.cycleRoles) }, diff --git a/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue b/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue index dd2ed57ef7e..04b05e39b91 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue @@ -129,6 +129,7 @@ import { useGraphClient } from 'web-client/src/composables' import CreateQuickLink from './Links/CreateQuickLink.vue' import { isLocationSpacesActive } from '../../../router' import { getLocaleFromLanguage } from 'web-pkg/src/helpers' +import { useIncomingParentShare } from '../../../composables/parentShare' export default defineComponent({ name: 'FileLinks', @@ -146,6 +147,7 @@ export default defineComponent({ return { ...useGraphClient(), + ...useIncomingParentShare(), hasSpaces: useCapabilitySpacesEnabled(), hasShareJail: useCapabilityShareJailEnabled(), hasResharing: useCapabilityFilesSharingResharing(), @@ -182,11 +184,6 @@ export default defineComponent({ return this.currentFileOutgoingLinks.find((link) => link.quicklink === true) }, - share() { - // the root share has an empty key in the shares tree. That's the reason why we retrieve the share by an empty key here - return this.sharesTree['/']?.find((s) => s.incoming) - }, - expirationDate() { const expireDate = this.capabilities.files_sharing.public.expire_date @@ -218,9 +215,9 @@ export default defineComponent({ }, availableRoleOptions() { - if (this.share?.incoming && this.canCreatePublicLinks) { + if (this.incomingParentShare && this.canCreatePublicLinks) { return LinkShareRoles.filterByBitmask( - parseInt(this.share.permissions), + parseInt(this.incomingParentShare.permissions), this.highlightedFile.isFolder, this.hasPublicLinkEditing, this.hasPublicLinkAliasSupport @@ -354,6 +351,9 @@ export default defineComponent({ return this.$route.params.storageId || null } }, + async mounted() { + await this.loadIncomingParentShare.perform(this.highlightedFile) + }, methods: { ...mapActions('Files', ['addLink', 'updateLink', 'removeLink']), ...mapActions(['showMessage', 'createModal', 'hideModal']), diff --git a/packages/web-app-files/src/composables/parentShare/index.ts b/packages/web-app-files/src/composables/parentShare/index.ts new file mode 100644 index 00000000000..9912152fdb3 --- /dev/null +++ b/packages/web-app-files/src/composables/parentShare/index.ts @@ -0,0 +1 @@ +export * from './useIncomingParentShare' diff --git a/packages/web-app-files/src/composables/parentShare/useIncomingParentShare.ts b/packages/web-app-files/src/composables/parentShare/useIncomingParentShare.ts new file mode 100644 index 00000000000..5c09b9fe658 --- /dev/null +++ b/packages/web-app-files/src/composables/parentShare/useIncomingParentShare.ts @@ -0,0 +1,47 @@ +import { buildShare } from '../../helpers/resources' +import { useStore } from 'web-pkg/src/composables' +import { useActiveLocation } from '../router' +import { isLocationSharesActive } from '../../router' +import { computed, ref, unref } from '@vue/composition-api' +import { useTask } from 'vue-concurrency' +import { clientService } from 'web-pkg/src/services' + +export function useIncomingParentShare() { + const store = useStore() + const incomingParentShare = ref(null) + const sharesTree = computed(() => store.state.Files.sharesTree) + const isSharedWithMeLocation = useActiveLocation(isLocationSharesActive, 'files-shares-with-me') + + const loadIncomingParentShare = useTask(function* (signal, resource) { + console.log('resource', resource) + let parentShare + for (const shares of Object.values(unref(sharesTree)) as any) { + parentShare = shares.find((s) => s.incoming) + if (parentShare) { + console.log('via incoming:', parentShare) + incomingParentShare.value = parentShare + return + } + } + + if (unref(isSharedWithMeLocation)) { + incomingParentShare.value = resource.share + console.log('via share obj', incomingParentShare.value) + return + } + + if (resource.shareId) { + const parentShare = yield clientService.owncloudSdk.shares.getShare(resource.shareId) + if (parentShare) { + incomingParentShare.value = buildShare(parentShare.shareInfo, resource, true) + console.log('via share id', incomingParentShare.value) + return + } + } + + console.log('No parent share') + incomingParentShare.value = null + }) + + return { loadIncomingParentShare, incomingParentShare } +} diff --git a/packages/web-app-files/src/helpers/resources.ts b/packages/web-app-files/src/helpers/resources.ts index 9a80d19b48f..1b9c6ac2b8f 100644 --- a/packages/web-app-files/src/helpers/resources.ts +++ b/packages/web-app-files/src/helpers/resources.ts @@ -70,6 +70,7 @@ export function buildResource(resource): Resource { permissions: (resource.fileInfo[DavProperty.Permissions] as string) || '', starred: resource.fileInfo[DavProperty.IsFavorite] !== '0', etag: resource.fileInfo[DavProperty.ETag], + shareId: resource.fileInfo[DavProperty.ShareId], sharePermissions: resource.fileInfo[DavProperty.SharePermissions], shareTypes: (function () { if (resource.fileInfo[DavProperty.ShareTypes]) { diff --git a/packages/web-app-files/tests/unit/components/SideBar/Shares/Collaborators/RoleDropdown.spec.js b/packages/web-app-files/tests/unit/components/SideBar/Shares/Collaborators/RoleDropdown.spec.js index 3fea667faba..f170cfe346e 100644 --- a/packages/web-app-files/tests/unit/components/SideBar/Shares/Collaborators/RoleDropdown.spec.js +++ b/packages/web-app-files/tests/unit/components/SideBar/Shares/Collaborators/RoleDropdown.spec.js @@ -274,7 +274,13 @@ function getMountOptions({ }, store: getStore(sharesTree), localVue, - stubs + stubs, + mocks: { + loadIncomingParentShare: { + perform: jest.fn() + }, + incomingParentShare: null + } } } diff --git a/packages/web-app-files/tests/unit/components/SideBar/Shares/FileLinks.spec.js b/packages/web-app-files/tests/unit/components/SideBar/Shares/FileLinks.spec.js index 24ead3fa009..ebce64d2f53 100644 --- a/packages/web-app-files/tests/unit/components/SideBar/Shares/FileLinks.spec.js +++ b/packages/web-app-files/tests/unit/components/SideBar/Shares/FileLinks.spec.js @@ -212,6 +212,10 @@ describe('FileLinks', () => { ...stubs }, mocks: { + loadIncomingParentShare: { + perform: jest.fn() + }, + incomingParentShare: null, $route: { params: {} }, @@ -230,6 +234,10 @@ describe('FileLinks', () => { localVue, store: store, mocks: { + loadIncomingParentShare: { + perform: jest.fn() + }, + incomingParentShare: null, $route: { params: {} }, diff --git a/packages/web-app-files/tests/unit/components/SideBar/Shares/FileShares.spec.js b/packages/web-app-files/tests/unit/components/SideBar/Shares/FileShares.spec.js index fb8c587ae79..328ede5280a 100644 --- a/packages/web-app-files/tests/unit/components/SideBar/Shares/FileShares.spec.js +++ b/packages/web-app-files/tests/unit/components/SideBar/Shares/FileShares.spec.js @@ -10,6 +10,7 @@ import Collaborators from '@/__fixtures__/collaborators' import { spaceRoleManager } from 'web-client/src/helpers/share' import * as reactivityComposables from 'web-pkg/src/composables/reactivity' import * as routerComposables from 'web-pkg/src/composables/router' +import { useActiveLocation } from '@files/src/composables/router' import FileShares from '@files/src/components/SideBar/Shares/FileShares.vue' import { buildSpace } from 'web-client/src/helpers' import { clientService } from 'web-pkg/src/services' @@ -26,6 +27,7 @@ localVue.use(GetTextPlugin, { jest.mock('web-pkg/src/composables/reactivity') jest.mock('web-pkg/src/composables/router') +jest.mock('@files/src/composables/router') const user = Users.alice const collaborators = [Collaborators[0], Collaborators[1]] @@ -275,6 +277,7 @@ const storeOptions = (data) => { function getMountedWrapper(data) { routerComposables.useRouteParam.mockReturnValue(() => storageId) + useActiveLocation.mockReturnValue(() => false) return mount(FileShares, { localVue, @@ -302,6 +305,7 @@ function getMountedWrapper(data) { function getShallowMountedWrapper(data, loading = false) { reactivityComposables.useDebouncedRef.mockImplementationOnce(() => loading) routerComposables.useRouteParam.mockReturnValue(() => storageId) + useActiveLocation.mockReturnValue(() => false) return shallowMount(FileShares, { localVue, diff --git a/packages/web-app-files/tests/unit/helpers/path.spec.js b/packages/web-app-files/tests/unit/helpers/path.spec.js index 0c2d9b88e4d..cc3f000bbbd 100644 --- a/packages/web-app-files/tests/unit/helpers/path.spec.js +++ b/packages/web-app-files/tests/unit/helpers/path.spec.js @@ -13,7 +13,7 @@ describe('build an array of parent paths from a provided path', () => { it('should prepend resulting paths with a "/" if none was given', () => { const paths = getParentPaths('a/b/c', false) - expect(paths).toEqual(['/a/b', '/a', '']) + expect(paths).toEqual(['/a/b', '/a']) }) it('should make no difference between "a/b/c" and "/a/b/c" with includeCurrent=false', () => { @@ -36,11 +36,11 @@ describe('build an array of parent paths from a provided path', () => { it('should not interpret a trailing slash as yet another path segment', () => { const paths = getParentPaths('/a/b/c/', true) - expect(paths).toEqual(['/a/b/c', '/a/b', '/a', '']) + expect(paths).toEqual(['/a/b/c', '/a/b', '/a']) }) it('should include the provided path in the result if includeCurrent=true', () => { const paths = getParentPaths('a/b/c', true) - expect(paths).toEqual(['/a/b/c', '/a/b', '/a', '']) + expect(paths).toEqual(['/a/b/c', '/a/b', '/a']) }) }) From a6c9390aaa2273979b49234092fe6b2aaffa29f2 Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Wed, 7 Sep 2022 15:00:28 +0200 Subject: [PATCH 03/17] Remove logs --- .../src/composables/parentShare/useIncomingParentShare.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packages/web-app-files/src/composables/parentShare/useIncomingParentShare.ts b/packages/web-app-files/src/composables/parentShare/useIncomingParentShare.ts index 5c09b9fe658..a3f587bbffd 100644 --- a/packages/web-app-files/src/composables/parentShare/useIncomingParentShare.ts +++ b/packages/web-app-files/src/composables/parentShare/useIncomingParentShare.ts @@ -13,12 +13,10 @@ export function useIncomingParentShare() { const isSharedWithMeLocation = useActiveLocation(isLocationSharesActive, 'files-shares-with-me') const loadIncomingParentShare = useTask(function* (signal, resource) { - console.log('resource', resource) let parentShare for (const shares of Object.values(unref(sharesTree)) as any) { parentShare = shares.find((s) => s.incoming) if (parentShare) { - console.log('via incoming:', parentShare) incomingParentShare.value = parentShare return } @@ -26,7 +24,6 @@ export function useIncomingParentShare() { if (unref(isSharedWithMeLocation)) { incomingParentShare.value = resource.share - console.log('via share obj', incomingParentShare.value) return } @@ -34,12 +31,10 @@ export function useIncomingParentShare() { const parentShare = yield clientService.owncloudSdk.shares.getShare(resource.shareId) if (parentShare) { incomingParentShare.value = buildShare(parentShare.shareInfo, resource, true) - console.log('via share id', incomingParentShare.value) return } } - console.log('No parent share') incomingParentShare.value = null }) From 84c0c2639b03ba6d141349caf2ec684f92f3dae6 Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Wed, 7 Sep 2022 15:25:39 +0200 Subject: [PATCH 04/17] Minor adjustment --- packages/web-app-files/src/helpers/path.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/web-app-files/src/helpers/path.js b/packages/web-app-files/src/helpers/path.js index a2a186f0699..678d3f36d60 100644 --- a/packages/web-app-files/src/helpers/path.js +++ b/packages/web-app-files/src/helpers/path.js @@ -21,7 +21,7 @@ export function getParentPaths(path = '', includeCurrent = false) { const paths = [] const sections = s.split('/') - if (includeCurrent && s) { + if (includeCurrent) { paths.push(s) } From afdcf208894e11129b423b90172212bd5aa25732 Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Thu, 8 Sep 2022 10:50:58 +0200 Subject: [PATCH 05/17] Add changelog item --- changelog/unreleased/bugfix-shares-tree-loading | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changelog/unreleased/bugfix-shares-tree-loading diff --git a/changelog/unreleased/bugfix-shares-tree-loading b/changelog/unreleased/bugfix-shares-tree-loading new file mode 100644 index 00000000000..5f55d6f35d2 --- /dev/null +++ b/changelog/unreleased/bugfix-shares-tree-loading @@ -0,0 +1,7 @@ +Bugfix: Hide share actions for space viewers/editors + +We now make sure that an empty string will never be used as key for the shares tree. This fixes several issues with wrong share permissions being loaded and also improves performance in folders. + +https://github.com/owncloud/web/issues/7506 +https://github.com/owncloud/web/issues/7593 +https://github.com/owncloud/web/pull/7580 From ceaf7dddade7969730f23ab4acdf7a69010579d0 Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Thu, 8 Sep 2022 11:13:51 +0200 Subject: [PATCH 06/17] Fix loading of share indicators --- .../src/components/SideBar/Shares/FileLinks.vue | 3 --- .../src/components/SideBar/Shares/FileShares.vue | 6 ------ packages/web-app-files/src/helpers/statusIndicators.js | 3 --- packages/web-app-files/src/mixins/actions/rename.js | 3 ++- 4 files changed, 2 insertions(+), 13 deletions(-) diff --git a/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue b/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue index 04b05e39b91..8aa2e983019 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue @@ -320,9 +320,6 @@ export default defineComponent({ return [] } - // remove root entry - parentPaths.pop() - parentPaths.forEach((parentPath) => { const shares = cloneStateObject(this.sharesTree[parentPath]) if (shares) { diff --git a/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue b/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue index 7efa9e41a50..0c80a0de3eb 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue @@ -203,9 +203,6 @@ export default { return [] } - // remove root entry - parentPaths.pop() - parentPaths.forEach((parentPath) => { const shares = this.sharesTree[parentPath] if (shares) { @@ -260,9 +257,6 @@ export default { return [] } - // remove root entry - parentPaths.pop() - parentPaths.forEach((parentPath) => { const shares = this.sharesTree[parentPath] if (shares) { diff --git a/packages/web-app-files/src/helpers/statusIndicators.js b/packages/web-app-files/src/helpers/statusIndicators.js index bd8fe0c587b..0aabedb685c 100644 --- a/packages/web-app-files/src/helpers/statusIndicators.js +++ b/packages/web-app-files/src/helpers/statusIndicators.js @@ -68,9 +68,6 @@ const shareTypesIndirect = (path, sharesTree) => { return [] } - // remove root entry - parentPaths.pop() - const shareTypes = {} parentPaths.forEach((parentPath) => { diff --git a/packages/web-app-files/src/mixins/actions/rename.js b/packages/web-app-files/src/mixins/actions/rename.js index 2c5ee39ed65..af3db8c9282 100644 --- a/packages/web-app-files/src/mixins/actions/rename.js +++ b/packages/web-app-files/src/mixins/actions/rename.js @@ -73,7 +73,8 @@ export default { const parentPaths = getParentPaths(resources[0].path, false).map((path) => { return prefix + path }) - parentResources = await this.$client.files.list(parentPaths[0], 1) + const parentPathRoot = parentPaths[0] ?? prefix + parentResources = await this.$client.files.list(parentPathRoot, 1) parentResources = parentResources.map(buildResource) } From 35fdcb9cdd77f2c52d76b2ae545e91d1c42afdcc Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Fri, 9 Sep 2022 11:06:00 +0200 Subject: [PATCH 07/17] Move sharesTree loading to the sidebar component --- .../unreleased/bugfix-shares-tree-loading | 9 ++- .../SideBar/Details/FileDetails.vue | 67 ++++++++--------- .../SideBar/Shares/Collaborators/ListItem.vue | 12 ---- .../Shares/Collaborators/RoleDropdown.vue | 18 +++-- .../components/SideBar/Shares/FileLinks.vue | 10 +-- .../components/SideBar/Shares/FileShares.vue | 43 ----------- .../components/SideBar/Shares/SharesPanel.vue | 71 ++++++++----------- .../src/components/SideBar/SideBar.vue | 43 ++++++++++- .../parentShare/useIncomingParentShare.ts | 8 --- .../web-app-files/src/helpers/resources.ts | 1 - 10 files changed, 118 insertions(+), 164 deletions(-) diff --git a/changelog/unreleased/bugfix-shares-tree-loading b/changelog/unreleased/bugfix-shares-tree-loading index 5f55d6f35d2..9b4adae320e 100644 --- a/changelog/unreleased/bugfix-shares-tree-loading +++ b/changelog/unreleased/bugfix-shares-tree-loading @@ -1,6 +1,11 @@ -Bugfix: Hide share actions for space viewers/editors +Bugfix: Shares tree loading -We now make sure that an empty string will never be used as key for the shares tree. This fixes several issues with wrong share permissions being loaded and also improves performance in folders. +We've improved loading of the shares tree: + +* It now happens more globally in the sidebar component instead of in each sidebar panel. +* Shares won't be loaded for resources without a path anymore. + +These changes massively improve the sidebar performance and fix several issues with (re-)share permissions. https://github.com/owncloud/web/issues/7506 https://github.com/owncloud/web/issues/7593 diff --git a/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue b/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue index e86ca9b1f5e..7bbfb4c8a2a 100644 --- a/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue +++ b/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue @@ -352,39 +352,40 @@ export default defineComponent({ watch: { file() { this.loadData() - this.refreshShareDetailsTree() - }, - sharesTree() { - // missing early return - this.sharedItem = null - this.shareIndicators = getIndicators(this.file, this.sharesTree) - const sharePathParentOrCurrent = this.getParentSharePath(this.file.path, this.sharesTree) - if (sharePathParentOrCurrent === null) { - return - } - const shares = this.sharesTree[sharePathParentOrCurrent]?.filter((s) => - ShareTypes.containsAnyValue( - [...ShareTypes.individuals, ...ShareTypes.unauthenticated], - [s.shareType] + }, + sharesTree: { + handler() { + // missing early return + this.sharedItem = null + this.shareIndicators = getIndicators(this.file, this.sharesTree) + const sharePathParentOrCurrent = this.getParentSharePath(this.file.path, this.sharesTree) + if (sharePathParentOrCurrent === null) { + return + } + const shares = this.sharesTree[sharePathParentOrCurrent]?.filter((s) => + ShareTypes.containsAnyValue( + [...ShareTypes.individuals, ...ShareTypes.unauthenticated], + [s.shareType] + ) ) - ) - if (shares.length === 0) { - return - } + if (shares.length === 0) { + return + } - this.sharedItem = shares[0] - this.sharedByName = this.sharedItem.owner?.name - this.sharedByDisplayName = this.sharedItem.owner?.displayName - if (this.sharedItem.owner?.additionalInfo) { - this.sharedByDisplayName += ' (' + this.sharedItem.owner.additionalInfo + ')' - } - this.sharedTime = this.sharedItem.stime - this.sharedParentDir = sharePathParentOrCurrent + this.sharedItem = shares[0] + this.sharedByName = this.sharedItem.owner?.name + this.sharedByDisplayName = this.sharedItem.owner?.displayName + if (this.sharedItem.owner?.additionalInfo) { + this.sharedByDisplayName += ' (' + this.sharedItem.owner.additionalInfo + ')' + } + this.sharedTime = this.sharedItem.stime + this.sharedParentDir = sharePathParentOrCurrent + }, + immediate: true } }, mounted() { this.loadData() - this.refreshShareDetailsTree() }, asyncComputed: { preview: { @@ -405,16 +406,8 @@ export default defineComponent({ } }, methods: { - ...mapActions('Files', ['loadPreview', 'loadVersions', 'loadSharesTree']), - async refreshShareDetailsTree() { - await this.loadSharesTree({ - client: this.$client, - path: this.file.path, - $gettext: this.$gettext, - storageId: this.file.fileId - }) - this.shareIndicators = getIndicators(this.file, this.sharesTree) - }, + ...mapActions('Files', ['loadPreview', 'loadVersions']), + getParentSharePath(childPath, shares) { let currentPath = childPath if (!currentPath) { diff --git a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/ListItem.vue b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/ListItem.vue index 713adcd5279..15996fb8d7d 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/ListItem.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/ListItem.vue @@ -217,13 +217,6 @@ export default defineComponent({ return this.$gettextInterpolate(translated, context) }, - shareExpirationText() { - const translated = this.$gettext('Expires %{ expiryDateRelative }') - return this.$gettextInterpolate(translated, { - expiryDateRelative: this.expirationDateRelative - }) - }, - screenreaderShareExpiration() { const translated = this.$gettext('Share expires %{ expiryDateRelative } (%{ expiryDate })') return this.$gettextInterpolate(translated, { @@ -258,11 +251,6 @@ export default defineComponent({ return this.sharedParentRoute?.params?.item.split('/').pop() }, - shareDetailsHelperContent() { - return { - text: this.$gettext('Invite persons or groups to access this file or folder.') - } - }, editDropDownToggleId() { return uuid.v4() }, diff --git a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue index c3aac0f1a23..469f56c9710 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/RoleDropdown.vue @@ -118,11 +118,11 @@ import { import * as uuid from 'uuid' import { defineComponent } from '@vue/runtime-core' import { PropType } from '@vue/composition-api' -import { useIncomingParentShare } from '../../../../composables/parentShare' export default defineComponent({ name: 'RoleDropdown', components: { RoleItem }, + inject: ['incomingParentShare'], props: { resource: { type: Object, @@ -148,9 +148,6 @@ export default defineComponent({ required: true } }, - setup() { - return { ...useIncomingParentShare() } - }, data() { return { selectedRole: null, @@ -193,9 +190,9 @@ export default defineComponent({ return SpacePeopleShareRoles.list() } - if (this.incomingParentShare && this.resourceIsSharable) { + if (this.incomingParentShare.value && this.resourceIsSharable) { return PeopleShareRoles.filterByBitmask( - parseInt(this.incomingParentShare.permissions), + parseInt(this.incomingParentShare.value.permissions), this.resource.isFolder, this.allowSharePermission, this.allowCustomSharing !== false @@ -205,8 +202,10 @@ export default defineComponent({ return PeopleShareRoles.list(this.resource.isFolder, this.allowCustomSharing !== false) }, availablePermissions() { - if (this.incomingParentShare && this.resourceIsSharable) { - return SharePermissions.bitmaskToPermissions(parseInt(this.incomingParentShare.permissions)) + if (this.incomingParentShare.value && this.resourceIsSharable) { + return SharePermissions.bitmaskToPermissions( + parseInt(this.incomingParentShare.value.permissions) + ) } return this.customPermissionsRole.permissions(this.allowSharePermission) }, @@ -223,8 +222,7 @@ export default defineComponent({ window.removeEventListener('keydown', this.cycleRoles) }, - async mounted() { - await this.loadIncomingParentShare.perform(this.resource) + mounted() { this.applyRoleAndPermissions() window.addEventListener('keydown', this.cycleRoles) }, diff --git a/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue b/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue index 8aa2e983019..6a4c0926f1b 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue @@ -129,7 +129,6 @@ import { useGraphClient } from 'web-client/src/composables' import CreateQuickLink from './Links/CreateQuickLink.vue' import { isLocationSpacesActive } from '../../../router' import { getLocaleFromLanguage } from 'web-pkg/src/helpers' -import { useIncomingParentShare } from '../../../composables/parentShare' export default defineComponent({ name: 'FileLinks', @@ -138,6 +137,7 @@ export default defineComponent({ DetailsAndEdit, NameAndCopy }, + inject: ['incomingParentShare'], setup() { const store = useStore() @@ -147,7 +147,6 @@ export default defineComponent({ return { ...useGraphClient(), - ...useIncomingParentShare(), hasSpaces: useCapabilitySpacesEnabled(), hasShareJail: useCapabilityShareJailEnabled(), hasResharing: useCapabilityFilesSharingResharing(), @@ -215,9 +214,9 @@ export default defineComponent({ }, availableRoleOptions() { - if (this.incomingParentShare && this.canCreatePublicLinks) { + if (this.incomingParentShare.value && this.canCreatePublicLinks) { return LinkShareRoles.filterByBitmask( - parseInt(this.incomingParentShare.permissions), + parseInt(this.incomingParentShare.value.permissions), this.highlightedFile.isFolder, this.hasPublicLinkEditing, this.hasPublicLinkAliasSupport @@ -348,9 +347,6 @@ export default defineComponent({ return this.$route.params.storageId || null } }, - async mounted() { - await this.loadIncomingParentShare.perform(this.highlightedFile) - }, methods: { ...mapActions('Files', ['addLink', 'updateLink', 'removeLink']), ...mapActions(['showMessage', 'createModal', 'hideModal']), diff --git a/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue b/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue index 0c80a0de3eb..073973137e8 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue @@ -188,35 +188,6 @@ export default { return this.collaborators.length > 0 }, - /** - * Returns all incoming shares, direct and indirect - * - * @return {Array.} list of incoming shares - */ - $_allIncomingShares() { - // direct incoming shares - const allShares = [...this.incomingShares] - - // indirect incoming shares - const parentPaths = getParentPaths(this.highlightedFile.path, true) - if (parentPaths.length === 0) { - return [] - } - - parentPaths.forEach((parentPath) => { - const shares = this.sharesTree[parentPath] - if (shares) { - shares.forEach((share) => { - if (share.incoming) { - allShares.push(share) - } - }) - } - }) - - return allShares - }, - collaborators() { return [...this.currentFileOutgoingCollaborators, ...this.indirectOutgoingShares] .sort(this.collaboratorsComparator) @@ -288,20 +259,6 @@ export default { const translatedFolder = this.$gettext("You don't have permission to share this folder.") return this.highlightedFile.type === 'file' ? translatedFile : translatedFolder }, - - currentUsersPermissions() { - if (this.$_allIncomingShares.length > 0) { - let permissions = 0 - - for (const share of this.$_allIncomingShares) { - permissions |= share.permissions - } - - return permissions - } - - return null - }, currentUserIsMemberOfSpace() { return this.currentSpace?.spaceMemberIds?.includes(this.user.uuid) }, diff --git a/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue b/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue index 1c19956d3d1..1c384996afc 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue @@ -18,11 +18,11 @@ import { computed, defineComponent, unref, watch } from '@vue/composition-api' import FileLinks from './FileLinks.vue' import FileShares from './FileShares.vue' import SpaceMembers from './SpaceMembers.vue' -import { mapActions, mapGetters, mapState } from 'vuex' -import { dirname } from 'path' +import { mapGetters, mapState } from 'vuex' import { useGraphClient } from 'web-client/src/composables' import { useTask } from 'vue-concurrency' import { useDebouncedRef, useStore } from 'web-pkg/src/composables' +import { useIncomingParentShare } from '../../../composables/parentShare' export default defineComponent({ name: 'SharesPanel', @@ -31,7 +31,12 @@ export default defineComponent({ FileShares, SpaceMembers }, - inject: ['activePanel'], + inject: ['activePanel', 'displayedItem'], + provide() { + return { + incomingParentShare: computed(() => this.incomingParentShare) + } + }, props: { showSpaceMembers: { type: Boolean, default: false }, showLinks: { type: Boolean, default: false } @@ -43,7 +48,13 @@ export default defineComponent({ ) const incomingSharesLoading = computed(() => store.state.Files.incomingSharesLoading) const sharesTreeLoading = computed(() => store.state.Files.sharesTreeLoading) - const sharesLoading = useDebouncedRef(true, 250) + const sharesLoading = useDebouncedRef( + currentFileOutgoingSharesLoading.value || + incomingSharesLoading.value || + sharesTreeLoading.value, + 250 + ) + watch([currentFileOutgoingSharesLoading, incomingSharesLoading, sharesTreeLoading], () => { sharesLoading.value = currentFileOutgoingSharesLoading.value || @@ -63,7 +74,12 @@ export default defineComponent({ }) }) - return { graphClient, loadSpaceMembersTask, sharesLoading } + return { + ...useIncomingParentShare(), + graphClient, + loadSpaceMembersTask, + sharesLoading + } }, computed: { ...mapGetters('Files', ['highlightedFile', 'currentFileOutgoingSharesLoading']), @@ -71,7 +87,10 @@ export default defineComponent({ }, watch: { sharesLoading: { - handler: function () { + handler: function (sharesLoading) { + if (!sharesLoading) { + this.loadIncomingParentShare.perform(this.displayedItem.value) + } if (this.loading || !unref(this.activePanel)) { return } @@ -83,49 +102,17 @@ export default defineComponent({ } this.$emit('scrollToElement', { element: this.$refs[ref].$el, panelName }) }) - } + }, + immediate: true }, highlightedFile: { handler: function (newItem, oldItem) { - if (oldItem !== newItem) { - this.$_reloadShares() - - if (this.showSpaceMembers) { - this.loadSpaceMembersTask.perform(this) - } + if (oldItem !== newItem && this.showSpaceMembers) { + this.loadSpaceMembersTask.perform(this) } }, immediate: true } - }, - methods: { - ...mapActions('Files', [ - 'loadCurrentFileOutgoingShares', - 'loadSharesTree', - 'loadIncomingShares' - ]), - $_reloadShares() { - this.loadCurrentFileOutgoingShares({ - client: this.$client, - graphClient: this.graphClient, - path: this.highlightedFile.path, - $gettext: this.$gettext, - storageId: this.highlightedFile.fileId, - resource: this.highlightedFile - }) - this.loadIncomingShares({ - client: this.$client, - path: this.highlightedFile.path, - $gettext: this.$gettext, - storageId: this.highlightedFile.fileId - }) - this.loadSharesTree({ - client: this.$client, - path: this.highlightedFile.path === '' ? '/' : dirname(this.highlightedFile.path), - $gettext: this.$gettext, - storageId: this.highlightedFile.fileId - }) - } } }) diff --git a/packages/web-app-files/src/components/SideBar/SideBar.vue b/packages/web-app-files/src/components/SideBar/SideBar.vue index ee907720037..3ae6cd23cfe 100644 --- a/packages/web-app-files/src/components/SideBar/SideBar.vue +++ b/packages/web-app-files/src/components/SideBar/SideBar.vue @@ -31,7 +31,7 @@ diff --git a/packages/web-app-files/src/components/SideBar/SideBar.vue b/packages/web-app-files/src/components/SideBar/SideBar.vue index 3ae6cd23cfe..bea0906b08b 100644 --- a/packages/web-app-files/src/components/SideBar/SideBar.vue +++ b/packages/web-app-files/src/components/SideBar/SideBar.vue @@ -54,6 +54,7 @@ import { import { bus } from 'web-pkg/src/instance' import { SideBarEventTopics } from '../../composables/sideBar' import _ from 'lodash' +import { useGraphClient } from 'web-client/src/composables' export default defineComponent({ components: { FileInfo, SpaceInfo, SideBar }, @@ -78,6 +79,7 @@ export default defineComponent({ setup() { const store = useStore() + const { graphClient } = useGraphClient() const closeSideBar = () => { bus.publish(SideBarEventTopics.close) @@ -105,7 +107,8 @@ export default defineComponent({ setActiveSideBarPanel, closeSideBar, destroySideBar, - focusSideBar + focusSideBar, + graphClient } }, @@ -236,6 +239,7 @@ export default defineComponent({ isLocationTrashActive(this.$router, 'files-trash-spaces-project') || this.highlightedFileIsSpace ) { + this.loadShares() this.selectedFile = { ...this.highlightedFile } return } @@ -271,7 +275,6 @@ export default defineComponent({ client: this.$client, graphClient: this.graphClient, path: this.highlightedFile.path, - $gettext: this.$gettext, storageId: this.highlightedFile.fileId, resource: this.highlightedFile }) From 45fbed2b902049017216e80fd68236456284779d Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Fri, 9 Sep 2022 14:06:04 +0200 Subject: [PATCH 12/17] Fix sidebar panel opening --- .../src/components/SideBar/Shares/SharesPanel.vue | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue b/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue index 1e1a076699f..fcedb0c2cf5 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue @@ -71,11 +71,12 @@ export default defineComponent({ }, watch: { sharesLoading: { - handler: function (sharesLoading) { + handler: function (sharesLoading, old) { if (!sharesLoading) { this.loadIncomingParentShare.perform(this.displayedItem.value) } - if (this.loading || !unref(this.activePanel)) { + // FIXME: !old can be removed as soon as https://github.com/owncloud/web/issues/7621 has been fixed + if (this.loading || !unref(this.activePanel) || !old) { return } this.$nextTick(() => { @@ -84,7 +85,7 @@ export default defineComponent({ if (!ref || !this.$refs[ref]) { return } - console.log('wa', this.$refs[ref].$el) + this.$emit('scrollToElement', { element: this.$refs[ref].$el, panelName }) }) }, From 511ddb6ee26aeada30fc13d49f9596148567c89b Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Fri, 9 Sep 2022 14:07:22 +0200 Subject: [PATCH 13/17] Remove unused method --- .../src/components/SideBar/Shares/SharesPanel.vue | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue b/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue index fcedb0c2cf5..efc30449b26 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue @@ -18,7 +18,7 @@ import { computed, defineComponent, unref, watch } from '@vue/composition-api' import FileLinks from './FileLinks.vue' import FileShares from './FileShares.vue' import SpaceMembers from './SpaceMembers.vue' -import { mapActions, mapGetters, mapState } from 'vuex' +import { mapGetters, mapState } from 'vuex' import { useDebouncedRef, useStore } from 'web-pkg/src/composables' import { useIncomingParentShare } from '../../../composables/parentShare' @@ -85,15 +85,11 @@ export default defineComponent({ if (!ref || !this.$refs[ref]) { return } - this.$emit('scrollToElement', { element: this.$refs[ref].$el, panelName }) }) }, immediate: true } - }, - methods: { - ...mapActions('Files', ['loadCurrentFileOutgoingShares']) } }) From 6faedfa04096f3e2640936f5beacef73c7a2b77f Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Fri, 9 Sep 2022 15:10:44 +0200 Subject: [PATCH 14/17] Fix e2e tests --- tests/e2e/support/objects/app-files/share/actions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/support/objects/app-files/share/actions.ts b/tests/e2e/support/objects/app-files/share/actions.ts index 3bc0838a2a8..5edb5591311 100644 --- a/tests/e2e/support/objects/app-files/share/actions.ts +++ b/tests/e2e/support/objects/app-files/share/actions.ts @@ -87,7 +87,7 @@ export const inviteMembers = async (args: inviteMembersArgs): Promise => { ]) await shareInputLocator.focus() await page.waitForSelector('.vs--open') - await page.locator(invitationInput).press('Enter') + await page.locator('.vs__dropdown-option').click() await page.locator(filesCollaboratorRolesSelector).click() await page.locator(util.format(collaboratorRoleItemSelector, role)).click() From fe53ea93cbe9626e45776425d424f126433110fc Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Mon, 12 Sep 2022 12:57:55 +0200 Subject: [PATCH 15/17] Apply small changes according to code review --- .../SideBar/Details/FileDetails.vue | 1 - .../components/SideBar/Shares/SharesPanel.vue | 25 +++++++------------ .../parentShare/useIncomingParentShare.ts | 4 +-- 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue b/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue index 64e327aa39b..7bbfb4c8a2a 100644 --- a/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue +++ b/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue @@ -380,7 +380,6 @@ export default defineComponent({ } this.sharedTime = this.sharedItem.stime this.sharedParentDir = sharePathParentOrCurrent - this.shareIndicators = getIndicators(this.file, this.sharesTree) }, immediate: true } diff --git a/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue b/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue index efc30449b26..38088aff08f 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/SharesPanel.vue @@ -14,12 +14,12 @@