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

Refactor collaborators to use helper classes and to map permissions #2373

Merged
merged 2 commits into from
Nov 21, 2019
Merged

Refactor collaborators to use helper classes and to map permissions #2373

merged 2 commits into from
Nov 21, 2019

Conversation

LukasHirt
Copy link
Contributor

@LukasHirt LukasHirt commented Nov 5, 2019 •
edited

Description

Introduced helper functions roleToBitmask and bitmaskToRole, separated collaborators into smaller components and small refactoring.

Related Issue

Motivation and Context

Cleaner code

How Has This Been Tested?

  • test environment: Manually
  1. Share with user
  2. Share with group
  3. Edit existing share

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:

@LukasHirt LukasHirt self-assigned this Nov 5, 2019
@LukasHirt LukasHirt added Status:Needs-Review Needs review from a maintainer and removed Status:In-Progress labels Nov 10, 2019
@LukasHirt
Copy link
Contributor Author

LukasHirt commented Nov 10, 2019 •
edited

@PVince81 Let's see how tests will behave now. PR should be ready for review. There is still some technical debt left but it is already getting really big so let's follow up on that in new PRs when we start working on the new "version" of collaborators.

I'm leaving all the commits in case it could maybe make it a little bit easier to review. When it will be ready to merge - I'll squash them.

@LukasHirt LukasHirt marked this pull request as ready for review November 10, 2019 21:23
@LukasHirt
Copy link
Contributor Author

Agh... need to bring back some references, classes and adjust the custom role to advanced permissions. Code still can be reviewed, I'll then do it in one commit together with potential changes.

PVince81
PVince81 reviewed Nov 13, 2019
View reviewed changes
apps/files/src/helpers/collaborators.js Outdated
let bitmask = 0

for (const permission of role.permissions) {
bitmask += permissionsBitmask[permission]
Copy link
Contributor

Choose a reason for hiding this comment

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

use |= to be safe

PVince81
PVince81 reviewed Nov 13, 2019
View reviewed changes
apps/files/src/components/Collaborators/Collaborator.vue Outdated
@@ -116,7 +83,11 @@ export default {
},

originalRole () {
return this.roles[this.collaborator.role].tag
if (this.collaborator.role.name === 'unknownRole') {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's just call the string "advancedRole" for consistency

Copy link
Contributor

Choose a reason for hiding this comment

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

or have it return null for the role

PVince81
PVince81 reviewed Nov 13, 2019
View reviewed changes
apps/files/src/components/Collaborators/RolesSelect.vue
},
methods: {
selectRole (role) {
this.$emit('roleSelected', role)
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: would it make sense to rename to "input" event ? as far as I remember Vue JS will automatically map the value to the ":model" directive if used

PVince81
PVince81 reviewed Nov 13, 2019
View reviewed changes
apps/files/src/helpers/collaborators.js Outdated

// Get bitmask of basic permissions
currentRole.permissions.forEach(permission => {
rolePermissionsBitmask += permissionsBitmask[permission]
Copy link
Contributor

Choose a reason for hiding this comment

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

use |=

PVince81
PVince81 reviewed Nov 13, 2019
View reviewed changes
apps/files/src/mixins/collaborators.js
@@ -78,6 +98,16 @@ export default {
.finally(_ => {
this.loading = false
})
},

collaboratorOptionChanged ({ role, permissions, propagate = true }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to have an attribute on the collaboratorRoles file that tells the name of the default role.

Then the parent view can pre-set the default role to this.selectedRole to avoid the "propagate = false" gymnastics.

Copy link
Contributor

Choose a reason for hiding this comment

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

also I assume that the roles API in the future will also provide a field for the default role (or we make it default to the one with the least permissions?)

Copy link
Contributor

Choose a reason for hiding this comment

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

=> later

PVince81
PVince81 reviewed Nov 13, 2019
View reviewed changes
apps/files/src/store/actions.js
if (s.permissions === '3' || s.permissions === '15' || s.permissions === '19' || s.permissions === '31') {
share.role = 'editor'
}
share.role = bitmaskToRole(s.permissions, file.type === 'folder')
Copy link
Contributor

Choose a reason for hiding this comment

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

if we inject the "roles" object as discussed above, the bitmaskToRole does not need to know any more if folder or not because said logic would be done by the caller.

Copy link
Contributor

Choose a reason for hiding this comment

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

=> later

@LukasHirt LukasHirt force-pushed the bugfix/sharing-bit-masking branch 2 times, most recently from b8a24c7 to 1fc797a Compare November 19, 2019 08:55
@LukasHirt
Added helper file for collaborators
20c88f5 
Added permissionsToRole function and restructured roles object

Fixed wrong type in jsdocs

Changed functions to interact with bitmask

Moved collaborators mixins into mixins folder and used roles helper file

Moved roles back into mixin

Move permissions bitmask to own file and introduce permissions sets for files and folders

Use custom permissions object

Split collaborators into smaller components

Move permissions bitmask back to collaborators helper file

Added object filter package and emit events from child components

Refactored default role

Added current role

Use smaller components in collaborator component

Added translations

Added docs and provide default function to return original strings

Added custom role

Implemented bitmaskToRole function

Bring bitmaskToRole into store

Fixed displaying of custom role

Fixed display of existing role

Fixed roleToBitmask function and add new collaborators method

Fixed passing of file into buildShare function

Fixed default role

Implement values of additional permissions

Add read permission to advanced role

Implemented change of collaborator

Fixed parentheses around group suffix)

Implemented requested changes

Renamed collaboratorRoles
@ownclouders
Copy link
Contributor

💥 Acceptance tests SharingPermissionsUsers failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/6549/

20191119-134437-276.png
20191119-134559-361.png
20191119-140404-333.png
20191119-140511-101.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests SharingPermissionsGroups failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/6549/

20191119-143601-100.png
20191119-143844-238.png
20191119-144538-011.png
20191119-144807-301.png
20191119-145948-987.png
20191119-145949-301.png
20191119-150217-260.png
20191119-150217-475.png
20191119-150444-482.png
20191119-150444-694.png
20191119-150711-871.png
20191119-150712-072.png
20191119-150959-910.png
20191119-151000-324.png
20191119-151247-742.png
20191119-151248-106.png

@PVince81
Copy link
Contributor

PVince81 commented Nov 19, 2019 •
edited by LukasHirt

@LukasHirt
Adjusted tests
098f71a 
Add permissions only if they exist for the role

Removed eslint rule for epect

Fixed epectations for custom role

Check permission for correct collaborator
@owncloud owncloud deleted a comment from ownclouders Nov 19, 2019
@LukasHirt
Copy link
Contributor Author

#2516

PVince81
PVince81 approved these changes Nov 21, 2019
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit bd2c97e into master Nov 21, 2019
@delete-merged-branch delete-merged-branch bot deleted the bugfix/sharing-bit-masking branch November 21, 2019 14:57
@LukasHirt LukasHirt mentioned this pull request Nov 25, 2019
Closed
10 tasks
@PVince81 PVince81 mentioned this pull request Nov 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 approved these changes

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

Assignees

@LukasHirt LukasHirt

Labels
Status:Needs-Review Needs review from a maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

collaborators custom role permission is not maintained
3 participants
@LukasHirt @ownclouders @PVince81