Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[full-ci] Implement people sharing for spaces #6455

Merged
merged 8 commits into from
Mar 7, 2022
Merged

[full-ci] Implement people sharing for spaces #6455

merged 8 commits into from
Mar 7, 2022

Conversation

JammingBen
Copy link
Collaborator

@JammingBen JammingBen commented Feb 18, 2022 •
edited
Loading

Description

Spaces can now be shared with other people. This change specifically includes:

  • listing all members who have access to a space (possible for all space members)
  • adding members to a space and giving them dedicated roles (possible for managers only)
  • editing the role of members (possible for managers only)
  • removing members from a space (possible for managers only)

Needs owncloud/owncloud-sdk#1013 to work properly. -> I've created a new alpha version for the SDK and updated the dependency in this PR. So running yarn is all you need to do.

Note that this PR only tackles the sharing of spaces via people. It does not include:

  • Link sharing
  • Group sharing
  • Sharing of resources within a space

Known issues

  • Querying each user via the graph API is really bad performance-wise. Needs Expand the drive GET-endpoints with user information ocis#3195 to be implemented first though
  • The permissions method of each role takes allowSharing as argument, because this permission is not determined by the roles, but by the given backend (?). However, with spaces, this changes. I've overcome this for now by always passing true to the method when dealing with shares... but there might be better solutions.
  • The displayed collaborators on the space front page might be wrong due to backend caching issues. The spaces overview works fine however.

Related Issue

Screenshots (if appropriate):

image

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@JammingBen JammingBen self-assigned this Feb 18, 2022
@JammingBen JammingBen changed the title Implement people sharing for spaces [WIP] Implement people sharing for spaces Feb 18, 2022
@delete-merged-branch delete-merged-branch bot deleted the branch master February 19, 2022 00:41
@JammingBen JammingBen changed the base branch from spaces-sidebar to master February 28, 2022 09:39
@JammingBen JammingBen mentioned this pull request Feb 28, 2022
Closed
8 tasks
@ownclouders
Copy link
Contributor

Results for oC10SharingIntGroupsToRoot https://drone.owncloud.com/owncloud/web/23100/28/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUISharingInternalGroupsToRootEdgeCases-shareWithGroupsEdgeCases_feature-L36.png

webUISharingInternalGroupsToRootEdgeCases-shareWithGroupsEdgeCases_feature-L36.png

💥 The acceptance tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for oC10SharingIntGroupsToRoot https://drone.owncloud.com/owncloud/web/23109/28/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUISharingInternalGroupsToRootEdgeCases-shareWithGroupsEdgeCases_feature-L36.png

webUISharingInternalGroupsToRootEdgeCases-shareWithGroupsEdgeCases_feature-L36.png

💥 The acceptance tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for oC10SharingIntGroupsToRoot https://drone.owncloud.com/owncloud/web/23114/28/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUISharingInternalGroupsToRootEdgeCases-shareWithGroupsEdgeCases_feature-L36.png

webUISharingInternalGroupsToRootEdgeCases-shareWithGroupsEdgeCases_feature-L36.png

💥 The acceptance tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for oC10SharingIntGroupsToRoot https://drone.owncloud.com/owncloud/web/23133/28/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUISharingInternalGroupsToRootEdgeCases-shareWithGroupsEdgeCases_feature-L36.png

webUISharingInternalGroupsToRootEdgeCases-shareWithGroupsEdgeCases_feature-L36.png

💥 The acceptance tests pipeline failed. The build has been cancelled.

@JammingBen JammingBen changed the title [WIP] Implement people sharing for spaces [WIP][full-ci] Implement people sharing for spaces Mar 1, 2022
@ownclouders
Copy link
Contributor

Results for oC10SharingIntGroupsToRoot https://drone.owncloud.com/owncloud/web/23139/28/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUISharingInternalGroupsToRootEdgeCases-shareWithGroupsEdgeCases_feature-L36.png

webUISharingInternalGroupsToRootEdgeCases-shareWithGroupsEdgeCases_feature-L36.png

@ownclouders
Copy link
Contributor

Results for oC10SharingIntGroups https://drone.owncloud.com/owncloud/web/23139/27/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUISharingInternalGroupsEdgeCases-shareWithGroupsEdgeCases_feature-L41.png

webUISharingInternalGroupsEdgeCases-shareWithGroupsEdgeCases_feature-L41.png

@ownclouders
Copy link
Contributor

Results for e2e-tests oCIS https://drone.owncloud.com/owncloud/web/23145/11/1

💥 To see the trace, please open the link in the console ...

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/23145/tracing/alice-can-share-this-weeks-meal-plan-with-all-parents-alice-2022-3-1-02-01-42.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/23145/tracing/alice-shares-file-to-brian-alice-2022-3-1-02-03-03.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/23145/tracing/alice-shares-file-to-brian-brian-2022-3-1-02-03-09.zip

@ownclouders
Copy link
Contributor

Results for oCISSharingInternal3 https://drone.owncloud.com/owncloud/web/23146/61/1
The following scenarios passed on retry:

  • webUISharingInternalUsersShareWithPage/shareWithUsers.feature:91

@ownclouders
Copy link
Contributor

Results for oCISSharingAndUpload https://drone.owncloud.com/owncloud/web/23146/66/1
The following scenarios passed on retry:

  • webUIUpload/upload.feature:69

@JammingBen JammingBen changed the title [WIP][full-ci] Implement people sharing for spaces [full-ci] Implement people sharing for spaces Mar 2, 2022
@JammingBen JammingBen marked this pull request as ready for review March 2, 2022 09:24
@ownclouders
Copy link
Contributor

Results for oCISFiles1 https://drone.owncloud.com/owncloud/web/23167/55/1
The following scenarios passed on retry:

  • webUIDeleteFilesFolders/deleteFilesFolders.feature:77
  • webUIDeleteFilesFolders/deleteFilesFolders.feature:253

kulmann
kulmann requested changes Mar 2, 2022
View reviewed changes
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a first try in the web ui, I feel like talking about members instead of people would be more appropriate. Meaning:
1: Members as title for the right sidebar panel
2: 1 member, 7 members in the link after the space title in the space root
Please discuss with @tbsbdr and @pmaier1

One more tiny detail: I think it would be more intuitive to have the three dots button for the spaces context menu after the title and only after that the members info. Looks a bit off, that the members info is in between the title and the three dots button.

When I try to share a space I get the following error message in the browser console (and nothing happens regarding sharing):

vendor-1671c65f.js:11 TypeError: i.shares.shareSpaceWithUser is not a function
    at rx.addShare (web-app-files-d3f6f72f.js:1:209447)
    at Array.<anonymous> (vendor-1671c65f.js:37:19210)
    at rx.dispatch (vendor-1671c65f.js:37:20947)
    at rx.dispatch (vendor-1671c65f.js:37:16415)
    at rx.i.dispatch (vendor-1671c65f.js:37:18321)
    at a.gx.forEach.r.<computed> (vendor-1671c65f.js:37:23607)
    at web-app-files-d3f6f72f.js:1:258071
    at vendor-1671c65f.js:37:33140
    at Rx._tryToStartAnother (vendor-1671c65f.js:37:32187)
    at vendor-1671c65f.js:37:33349

@tbsbdr
Copy link
Contributor

tbsbdr commented Mar 2, 2022

member/s here ... and here:
image

@JammingBen
Copy link
Collaborator Author

When I try to share a space I get the following error message in the browser console (and nothing happens regarding sharing)

Sounds like you didn't link owncloud/owncloud-sdk#1013. My plan is to create a new alpha release once the PR is merged and add it here. But I'm still waiting for feedback from the OCIS team.

@tbsbdr
Copy link
Contributor

tbsbdr commented Mar 2, 2022 •
edited
Loading

Proposal to make the Headline-3Dots-Memberscount-line more appealing

  • Full width of the description
  • 3-dots next to Space-Title
  • Members Count aligned to the left
  • Space-image: ratio 16:9 - height is fixed

Please speak up, if you don't agree
Single Space - full frontpage-right-icon

@tbsbdr
Copy link
Contributor

tbsbdr commented Mar 2, 2022 •
edited
Loading

updated screenshot above: Members-count has the icon now on the left.

@JammingBen JammingBen requested a review from kulmann March 3, 2022 10:32
kulmann
kulmann requested changes Mar 4, 2022
View reviewed changes
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't feel intimidated by the number of change requests - they should be easily resolved. This PR is in really good shape and a huge step forward. Awesome! 🥳

packages/web-app-files/src/components/SideBar/Shares/InviteCollaborator/RecipientContainer.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/components/SideBar/Details/SpaceDetails.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/components/SideBar/Details/SpaceDetails.vue Show resolved Hide resolved
...es/web-app-files/src/components/SideBar/Shares/InviteCollaborator/InviteCollaboratorForm.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/helpers/share/role.ts Outdated Show resolved Hide resolved
packages/web-app-files/src/components/SideBar/Shares/SpaceShares.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/components/SideBar/Shares/SpaceShares.vue Show resolved Hide resolved
packages/web-app-files/src/components/SideBar/Shares/SpaceShares.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/views/spaces/Project.vue Show resolved Hide resolved
packages/web-app-files/src/views/spaces/Projects.vue
@@ -361,6 +378,11 @@ export default {
return 'state-trashed'
}
return ''
},
async openSidebarSharePanel(space) {
await this.closeSidebar()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it needed to close the sidebar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know why I added it, it's not :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I found the issue again, it is. It seems to be necessary when toggling through the share panel in the spaces overview (via the people-indicator). Otherwise, the share panel does not get updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that an indicator for data not properly watched in the sharing panel? Can you look into that after merge? Closing and re-opening the sidebar feels a bit glitchy and might cause side effects in the future.

@kulmann
Copy link
Member

kulmann commented Mar 4, 2022 •
edited
Loading

owncloud-sdk v2.1.0-alpha.3 has been merged to master in the meantime. Instead of rebasing, it should be sufficient to cherry-pick the commit e36e14711fcd5b758ac998c484d121d11eaff4b2 into your branch.

@sonarcloud
Copy link

sonarcloud bot commented Mar 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

72.9% 72.9% Coverage
0.0% 0.0% Duplication

@JammingBen JammingBen requested a review from kulmann March 7, 2022 09:01
kulmann
kulmann approved these changes Mar 7, 2022
View reviewed changes
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! 😍

packages/web-app-files/src/components/SideBar/Shares/RoleDropdown.vue Show resolved Hide resolved
packages/web-app-files/src/views/spaces/Projects.vue
@@ -361,6 +378,11 @@ export default {
return 'state-trashed'
}
return ''
},
async openSidebarSharePanel(space) {
await this.closeSidebar()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that an indicator for data not properly watched in the sharing panel? Can you look into that after merge? Closing and re-opening the sidebar feels a bit glitchy and might cause side effects in the future.

@kulmann kulmann merged commit eb10385 into master Mar 7, 2022
@delete-merged-branch delete-merged-branch bot deleted the spaces-collaborators branch March 7, 2022 11:03
@JammingBen JammingBen mentioned this pull request Mar 7, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kulmann kulmann kulmann approved these changes

@pascalwengerter pascalwengerter Awaiting requested review from pascalwengerter

@fschade fschade Awaiting requested review from fschade

@AlexAndBear AlexAndBear Awaiting requested review from AlexAndBear

Assignees

@JammingBen JammingBen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JammingBen @ownclouders @tbsbdr @kulmann