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.

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

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

Merged
merged 8 commits into from
Mar 7, 2022
Merged

Conversation

JammingBen
Copy link
Contributor

@JammingBen JammingBen commented Feb 18, 2022

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
@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.

@JammingBen JammingBen force-pushed the spaces-collaborators branch from 64546a0 to cd90c58 Compare March 1, 2022 11:20
@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 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 force-pushed the spaces-collaborators branch from bbf4a57 to d0717e9 Compare March 2, 2022 09:17
@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

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

tbsbdr commented Mar 2, 2022

member/s here ... and here:
image

@JammingBen
Copy link
Contributor 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

tbsbdr commented Mar 2, 2022

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

tbsbdr commented Mar 2, 2022

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

@JammingBen JammingBen force-pushed the spaces-collaborators branch from 2cc64d5 to 16326e4 Compare March 3, 2022 10:25
@JammingBen JammingBen requested a review from kulmann March 3, 2022 10:32
@JammingBen JammingBen force-pushed the spaces-collaborators branch from 16326e4 to 7da85df Compare March 4, 2022 12:07
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/helpers/share/role.ts Outdated Show resolved Hide resolved
@@ -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
Contributor 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
Contributor 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

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.

@JammingBen JammingBen force-pushed the spaces-collaborators branch from 7da85df to 7a4b939 Compare March 7, 2022 08:51
@sonarqubecloud
Copy link

sonarqubecloud 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
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! 😍

@@ -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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants