From 3063fc247bb5e6738619d61ba896cf9c51e8c636 Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Mon, 23 May 2022 10:10:56 +0200 Subject: [PATCH] use ocis storageId whenever possible use roles whenever possible refactor share loading out of link and share panel component into parent component and load the data only once use fileId for public link sharing cleanup expected failures get rid of bug demonstration test add gherkin debug helpers bump sdk bump ocis --- .drone.env | 2 +- .drone.star | 1 + .../unreleased/enhancement-ocis-resharing | 6 + .../SideBar/Details/FileDetails.vue | 2 +- .../InviteCollaboratorForm.vue | 13 +- .../SideBar/Shares/Collaborators/ListItem.vue | 3 +- .../components/SideBar/Shares/FileLinks.vue | 25 +--- .../components/SideBar/Shares/FileShares.vue | 39 +---- .../components/SideBar/Shares/SharesPanel.vue | 55 +++++++ packages/web-app-files/src/store/actions.js | 136 +++++++----------- .../SideBar/Shares/FileShares.spec.js | 12 -- .../SideBar/Shares/SharesPanel.spec.js | 49 +++++++ .../tests/unit/store/actions.spec.js | 6 +- .../stepDefinitions/debugContext.js | 5 + .../e2e/cucumber/steps/app-files/resource.ts | 8 +- tests/e2e/support/api/graph/user.ts | 1 + tests/e2e/support/environment/actor/shared.ts | 3 +- 17 files changed, 195 insertions(+), 171 deletions(-) create mode 100644 changelog/unreleased/enhancement-ocis-resharing create mode 100644 packages/web-app-files/tests/unit/components/SideBar/Shares/SharesPanel.spec.js create mode 100644 tests/acceptance/stepDefinitions/debugContext.js diff --git a/.drone.env b/.drone.env index d8592750807..d99e3e42c7b 100644 --- a/.drone.env +++ b/.drone.env @@ -1,3 +1,3 @@ # The version of OCIS to use in pipelines that test against OCIS -OCIS_COMMITID=49541e6cc244f933e1d42b0462664eb6f1ab4ad4 +OCIS_COMMITID=2e79be6bfd92d373eb63039e66354fe9d13aaae0 OCIS_BRANCH=master diff --git a/.drone.star b/.drone.star index 32ead7040b3..2d27bf8ab1d 100644 --- a/.drone.star +++ b/.drone.star @@ -2196,6 +2196,7 @@ def ocisService(): "STORAGE_USERS_DRIVER_OWNCLOUD_DATADIR": "/srv/app/tmp/ocis/owncloud/data", "WEB_ASSET_PATH": "%s/dist" % dir["web"], "WEB_UI_CONFIG": "/srv/config/drone/config-ocis.json", + "FRONTEND_ENABLE_RESHARING": "true", }, "commands": [ "cd %s/ocis-build" % dir["base"], diff --git a/changelog/unreleased/enhancement-ocis-resharing b/changelog/unreleased/enhancement-ocis-resharing new file mode 100644 index 00000000000..e369f600382 --- /dev/null +++ b/changelog/unreleased/enhancement-ocis-resharing @@ -0,0 +1,6 @@ +Enhancement: Re-sharing for ocis + +We've enhanced web to be able to re-share resources for ocis, which works from sharing jails, project and personal spaces. +Beside that we also send roles, space-ref and path as separate values to the sharing api which simplifies the usage of it. + +https://github.com/owncloud/web/pull/7086 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 a82e561adc2..0addd1de11c 100644 --- a/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue +++ b/packages/web-app-files/src/components/SideBar/Details/FileDetails.vue @@ -393,7 +393,7 @@ export default defineComponent({ client: this.$client, path: this.file.path, $gettext: this.$gettext, - ...(this.currentStorageId && { storageId: this.currentStorageId }) + storageId: this.file.fileId }) this.shareIndicators = getIndicators(this.file, this.sharesTree) }, diff --git a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/InviteCollaborator/InviteCollaboratorForm.vue b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/InviteCollaborator/InviteCollaboratorForm.vue index 71ae9934cd7..aafd9b9a61a 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/Collaborators/InviteCollaborator/InviteCollaboratorForm.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/Collaborators/InviteCollaborator/InviteCollaboratorForm.vue @@ -186,9 +186,6 @@ export default { return this.$gettext('Invite') }, - currentStorageId() { - return this.$route.params.storageId - }, resourceIsSpace() { return this.highlightedFile.type === 'space' }, @@ -326,13 +323,6 @@ export default { this.selectedRole.permissions(this.hasResharing || this.resourceIsSpace) ) - let storageId - if (this.resourceIsSpace) { - storageId = this.highlightedFile.id - } else if (this.currentStorageId) { - storageId = this.currentStorageId - } - this.addShare({ client: this.$client, graphClient: this.graphClient, @@ -342,8 +332,9 @@ export default { displayName: collaborator.label, shareType: collaborator.value.shareType, permissions: bitmask, + role: this.selectedRole, expirationDate: this.expirationDate, - storageId + storageId: this.highlightedFile.fileId || this.highlightedFile.id }) }) ) 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 1e6c4636827..710d100d453 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 @@ -309,7 +309,8 @@ export default { graphClient: this.graphClient, share: this.share, permissions: bitmask, - expirationDate: expirationDate || '' + expirationDate: expirationDate || '', + role }) } } 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 a32c84dcaea..10403c7eed4 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue @@ -118,9 +118,9 @@ import { getParentPaths } from '../../../helpers/path' import { ShareTypes, LinkShareRoles, SharePermissions } from '../../../helpers/share' import { cloneStateObject } from '../../../helpers/store' import { showQuickLinkPasswordModal } from '../../../quickActions' -import CreateQuickLink from './Links/CreateQuickLink.vue' import DetailsAndEdit from './Links/DetailsAndEdit.vue' import NameAndCopy from './Links/NameAndCopy.vue' +import CreateQuickLink from './Links/CreateQuickLink.vue' export default defineComponent({ name: 'FileLinks', @@ -336,25 +336,8 @@ export default defineComponent({ return this.$route.params.storageId || null } }, - watch: { - highlightedFile(newItem, oldItem) { - if (oldItem !== newItem) { - this.reloadLinks() - } - } - }, - mounted() { - this.reloadLinks() - }, - methods: { - ...mapActions('Files', [ - 'addLink', - 'updateLink', - 'removeLink', - 'loadSharesTree', - 'loadCurrentFileOutgoingShares' - ]), + ...mapActions('Files', ['addLink', 'updateLink', 'removeLink']), ...mapActions(['showMessage', 'createModal', 'hideModal']), toggleLinkListCollapsed() { @@ -485,9 +468,9 @@ export default defineComponent({ permissions: link.permissions, quicklink: link.quicklink, name: link.name, + spaceRef: this.highlightedFile.fileId, ...(this.currentStorageId && { - storageId: this.currentStorageId, - spaceRef: `${this.currentStorageId}${this.highlightedFile.path}` + storageId: this.currentStorageId }) } }, 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 97c8fcd2d75..94bfc7eba8b 100644 --- a/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue +++ b/packages/web-app-files/src/components/SideBar/Shares/FileShares.vue @@ -61,7 +61,6 @@ diff --git a/packages/web-app-files/src/store/actions.js b/packages/web-app-files/src/store/actions.js index fc3385a210e..1dea3bf503c 100644 --- a/packages/web-app-files/src/store/actions.js +++ b/packages/web-app-files/src/store/actions.js @@ -14,7 +14,7 @@ import { $gettext, $gettextInterpolate } from '../gettext' import { loadPreview, move, copy } from '../helpers/resource' import { avatarUrl } from '../helpers/user' import { has } from 'lodash-es' -import { ShareTypes, SpacePeopleShareRoles } from '../helpers/share' +import { ShareTypes } from '../helpers/share' import { sortSpaceMembers } from '../helpers/space' import get from 'lodash-es/get' import { ClipboardActions } from '../helpers/clipboardActions' @@ -243,11 +243,6 @@ export default { context.commit('CURRENT_FILE_OUTGOING_SHARES_ERROR', null) context.commit('CURRENT_FILE_OUTGOING_SHARES_LOADING', true) - let spaceRef - if (storageId) { - spaceRef = `${storageId}${path}` - } - if (resource?.type === 'space') { const promises = [] const spaceMembers = [] @@ -264,7 +259,7 @@ export default { } promises.push( - client.shares.getShares(path, { reshares: true, spaceRef }).then((data) => { + client.shares.getShares(path, { reshares: true, spaceRef: storageId }).then((data) => { for (const element of data) { spaceLinks.push( buildShare( @@ -294,7 +289,7 @@ export default { // see https://owncloud.dev/owncloud-sdk/Shares.html client.shares - .getShares(path, { reshares: true, spaceRef }) + .getShares(path, { reshares: true, spaceRef: storageId }) .then((data) => { context.commit( 'CURRENT_FILE_OUTGOING_SHARES_SET', @@ -342,70 +337,56 @@ export default { context.commit('INCOMING_SHARES_LOADING', false) }) }, - changeShare( + async changeShare( { commit, getters, rootGetters }, - { client, graphClient, share, permissions, expirationDate } + { client, graphClient, share, permissions, expirationDate, role } ) { - const params = { - permissions: permissions, - expireDate: expirationDate + if (!permissions && !role) { + throw new Error('Nothing changed') } - if (!params.permissions) { - return new Promise((resolve, reject) => { - reject(new Error('Nothing changed')) + if (share.shareType === ShareTypes.space.value) { + await client.shares.shareSpaceWithUser('', share.collaborator.name, share.id, { + permissions, + role: role.name }) - } - if (share.shareType === ShareTypes.space.value) { - return new Promise((resolve, reject) => { - client.shares - .shareSpaceWithUser('', share.collaborator.name, share.id, { - permissions - }) - .then(() => { - const role = SpacePeopleShareRoles.getByBitmask(permissions) - const shareObj = { - role: role.name, - onPremisesSamAccountName: share.collaborator.name, - displayName: share.collaborator.displayName - } - const updatedShare = buildSpaceShare(shareObj, share.id) - commit('CURRENT_FILE_OUTGOING_SHARES_UPSERT', updatedShare) - - graphClient.drives.getDrive(share.id).then((response) => { - const space = buildSpace(response.data) - commit('UPDATE_RESOURCE_FIELD', { - id: share.id, - field: 'spaceRoles', - value: space.spaceRoles - }) - }) + const updatedShare = buildSpaceShare( + { + role: role.name, + onPremisesSamAccountName: share.collaborator.name, + displayName: share.collaborator.displayName + }, + share.id + ) - resolve(updatedShare) - }) - .catch((e) => { - reject(e) - }) + commit('CURRENT_FILE_OUTGOING_SHARES_UPSERT', updatedShare) + + const { data: drive } = await graphClient.drives.getDrive(share.id) + const space = buildSpace(drive) + commit('UPDATE_RESOURCE_FIELD', { + id: share.id, + field: 'spaceRoles', + value: space.spaceRoles }) + + return } - return new Promise((resolve, reject) => { - client.shares - .updateShare(share.id, params) - .then((updatedShare) => { - const share = buildCollaboratorShare( - updatedShare.shareInfo, - getters.highlightedFile, - allowSharePermissions(rootGetters) - ) - commit('CURRENT_FILE_OUTGOING_SHARES_UPSERT', share) - resolve(share) - }) - .catch((e) => { - reject(e) - }) + const updatedShare = await client.shares.updateShare(share.id, { + role: role.name, + permissions, + expireDate: expirationDate }) + + commit( + 'CURRENT_FILE_OUTGOING_SHARES_UPSERT', + buildCollaboratorShare( + updatedShare.shareInfo, + getters.highlightedFile, + allowSharePermissions(rootGetters) + ) + ) }, addShare( context, @@ -416,22 +397,19 @@ export default { shareWith, shareType, permissions, + role, expirationDate, storageId, displayName } ) { - let spaceRef - if (storageId) { - spaceRef = `${storageId}${path}` - } - if (shareType === ShareTypes.group.value) { client.shares .shareFileWithGroup(path, shareWith, { - permissions: permissions, - expirationDate: expirationDate, - spaceRef + permissions, + role: role.name, + expirationDate, + spaceRef: storageId }) .then((share) => { context.commit( @@ -462,10 +440,10 @@ export default { if (shareType === ShareTypes.space.value) { client.shares .shareSpaceWithUser(path, shareWith, storageId, { - permissions + permissions, + role: role.name }) .then(() => { - const role = SpacePeopleShareRoles.getByBitmask(permissions) const shareObj = { role: role.name, onPremisesSamAccountName: shareWith, @@ -505,10 +483,11 @@ export default { const remoteShare = shareType === ShareTypes.remote.value client.shares .shareFileWithUser(path, shareWith, { - permissions: permissions, remoteUser: remoteShare, - expirationDate: expirationDate, - spaceRef + permissions, + role: role.name, + expirationDate, + spaceRef: storageId }) .then((share) => { context.commit( @@ -591,11 +570,6 @@ export default { return Promise.resolve() } - let spaceRef - if (storageId) { - spaceRef = `${storageId}${path}` - } - // remove last entry which is the root folder parentPaths.pop() @@ -613,7 +587,7 @@ export default { shareQueriesPromises.push( shareQueriesQueue.add(() => client.shares - .getShares(queryPath, { reshares: true, spaceRef }) + .getShares(queryPath, { reshares: true, spaceRef: storageId }) .then((data) => { data.forEach((element) => { sharesTree[queryPath].push({ @@ -638,7 +612,7 @@ export default { shareQueriesPromises.push( shareQueriesQueue.add(() => client.shares - .getShares(queryPath, { shared_with_me: true, spaceRef }) + .getShares(queryPath, { shared_with_me: true, spaceRef: storageId }) .then((data) => { data.forEach((element) => { sharesTree[queryPath].push({ 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 8a8e5fd6e9a..a78caebd9f4 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 @@ -93,18 +93,6 @@ describe('FileShares', () => { await wrapper.vm.$nextTick() expect(spyOnCollaboratorDeleteTrigger).toHaveBeenCalledTimes(1) }) - it('reloads shares if highlighted file is changed', async () => { - const spyOnReloadShares = jest - .spyOn(FileShares.methods, '$_reloadShares') - .mockImplementation() - const wrapper = getMountedWrapper({ - user, - outgoingCollaborators: collaborators - }) - wrapper.vm.$store.commit('Files/SET_HIGHLIGHTED_FILE', { name: 'testfile2' }) - await wrapper.vm.$nextTick() - expect(spyOnReloadShares).toHaveBeenCalledTimes(1) - }) it('correctly passes the shared parent route to the collaborator list item', () => { const wrapper = getShallowMountedWrapper({ user, diff --git a/packages/web-app-files/tests/unit/components/SideBar/Shares/SharesPanel.spec.js b/packages/web-app-files/tests/unit/components/SideBar/Shares/SharesPanel.spec.js new file mode 100644 index 00000000000..c31f11c18d9 --- /dev/null +++ b/packages/web-app-files/tests/unit/components/SideBar/Shares/SharesPanel.spec.js @@ -0,0 +1,49 @@ +import { createLocalVue, mount } from '@vue/test-utils' +import GetTextPlugin from 'vue-gettext' +import Vuex from 'vuex' +import DesignSystem from 'owncloud-design-system' +import SharesPanel from '@files/src/components/SideBar/Shares/SharesPanel.vue' + +const localVue = createLocalVue() +localVue.use(DesignSystem) +localVue.use(Vuex) +localVue.use(GetTextPlugin, { + translations: 'does-not-matter.json', + silent: true +}) + +jest.mock('web-pkg/src/composables/reactivity') +describe('SharesPanel', () => { + it('reloads shares if highlighted file is changed', async () => { + const spyOnReloadShares = jest.spyOn(SharesPanel.methods, '$_reloadShares').mockImplementation() + const wrapper = mount(SharesPanel, { + localVue, + store: new Vuex.Store({ + modules: { + Files: { + namespaced: true, + state: { + highlightedFile: { name: '1' } + }, + getters: { + highlightedFile: (state) => { + return state.highlightedFile + } + }, + mutations: { + SET_HIGHLIGHTED_FILE(state, file) { + state.highlightedFile = file + } + } + } + } + }), + stubs: { + 'file-shares': true + } + }) + wrapper.vm.$store.commit('Files/SET_HIGHLIGHTED_FILE', { name: '2' }) + await wrapper.vm.$nextTick() + expect(spyOnReloadShares).toHaveBeenCalledTimes(2) + }) +}) diff --git a/packages/web-app-files/tests/unit/store/actions.spec.js b/packages/web-app-files/tests/unit/store/actions.spec.js index 1dcffb8e5ce..63685553ea8 100644 --- a/packages/web-app-files/tests/unit/store/actions.spec.js +++ b/packages/web-app-files/tests/unit/store/actions.spec.js @@ -108,7 +108,8 @@ describe('vuex store actions', () => { client: clientMock, graphClient: graphClientMock, share: dataSet.share, - permissions: 1, + permissions: spaceRoleManager.bitmask(false), + role: spaceRoleManager, expirationDate: null }) @@ -128,7 +129,8 @@ describe('vuex store actions', () => { graphClient: graphClientMock, shareType: dataSet.shareType, storageId: dataSet.storageId, - permissions: 1, + permissions: spaceRoleManager.bitmask(false), + role: spaceRoleManager, expirationDate: null }) diff --git a/tests/acceptance/stepDefinitions/debugContext.js b/tests/acceptance/stepDefinitions/debugContext.js new file mode 100644 index 00000000000..45974137941 --- /dev/null +++ b/tests/acceptance/stepDefinitions/debugContext.js @@ -0,0 +1,5 @@ +const { When } = require('@cucumber/cucumber') + +When('pause for {int}', async function (duration) { + await new Promise((resolve) => setTimeout(resolve, duration)) +}) diff --git a/tests/e2e/cucumber/steps/app-files/resource.ts b/tests/e2e/cucumber/steps/app-files/resource.ts index 87970eaffcb..983eefc59f2 100644 --- a/tests/e2e/cucumber/steps/app-files/resource.ts +++ b/tests/e2e/cucumber/steps/app-files/resource.ts @@ -1,4 +1,5 @@ import { DataTable, When, Then } from '@cucumber/cucumber' +import path from 'path' import { World } from '../../environment' import { objects } from '../../../support' import { expect } from '@playwright/test' @@ -223,13 +224,14 @@ export const processDownload = async ( } else { expect(downloads.length).toBe(1) downloads.forEach((download) => { + const { name } = path.parse(download.suggestedFilename()) if (config.ocis) { - expect(download.suggestedFilename()).toBe('download.tar') + expect(name).toBe('download') } else { if (parentFolder) { - expect(download.suggestedFilename()).toBe(parentFolder + '.zip') + expect(name).toBe(parentFolder) } else { - expect(download.suggestedFilename()).toBe('download.zip') + expect(name).toBe('download') } } }) diff --git a/tests/e2e/support/api/graph/user.ts b/tests/e2e/support/api/graph/user.ts index be3bbb5d90e..500023588cf 100644 --- a/tests/e2e/support/api/graph/user.ts +++ b/tests/e2e/support/api/graph/user.ts @@ -26,6 +26,7 @@ export const createUser = async ({ user, admin }: { user: User; admin: User }): body, user: admin }) + checkResponseStatus(response, 'Failed while creating user') return user } diff --git a/tests/e2e/support/environment/actor/shared.ts b/tests/e2e/support/environment/actor/shared.ts index 67720f87083..2ecd519d4fb 100644 --- a/tests/e2e/support/environment/actor/shared.ts +++ b/tests/e2e/support/environment/actor/shared.ts @@ -20,7 +20,8 @@ export interface ActorOptions extends ActorsOptions { export const buildBrowserContextOptions = (options: ActorOptions): BrowserContextOptions => { const contextOptions: BrowserContextOptions = { acceptDownloads: options.context.acceptDownloads, - ignoreHTTPSErrors: true + ignoreHTTPSErrors: true, + locale: 'en-US' } if (options.context.reportVideo) {