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

Documents in a folder should be shared with its group by default #1548

Closed

Conversation

ClaireLozano
Copy link
Contributor

@ClaireLozano ClaireLozano commented Mar 6, 2018

Issue : #1407


This change is Reviewable

Copy link
Member

@brecke brecke left a comment

Choose a reason for hiding this comment

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

Not displaying expected behaviour

var createFn = _.partial(ContentDAO.Content.createContent, contentId, revisionId, ctx.user().id, resourceSubType, displayName, description, visibility, otherValues, revisionData);
ResourceActions.create(ctx, roles, createFn, function(err, content, revision, memberChangeInfo) {
// If the content is on a folder, add those members to the content created
_getFolderMembers(ctx, folders, roles, resourceSubType, function(err, roles) {
Copy link
Member

@brecke brecke Mar 26, 2018

Choose a reason for hiding this comment

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

aha! here's the problem. These are the roles that are currently valid, but this is not what we want, is it? we want the permissions that have been defined by the user in the "shared with" form. Those are the ones we want.

A good strategy would be to pick the current ones and then filter them using the form. That way we would have the folder roles, e.g.

{ 'u:up:SkmX6OL9M': 'manager', 'u:uc:HyDW6O8qf': 'editor' }

and then compare it to what comes through the form. For instance, if one user has been removed from the input field inside the modal box, we take it out and it becomes

{ 'u:up:SkmX6OL9M': 'manager' }

See below 👇

Copy link
Member

@brecke brecke Mar 26, 2018

Choose a reason for hiding this comment

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

The POST that is sent when one creates the document goes like this (in my local scenario):

resourceSubType:collabdoc
displayName:My private fifth document
description:
visibility:private
managers:u:up:SkmX6OL9M
managers:u:up:SkmX6OL9M
folders:f:up:rkqGJYUqG

So you see, that's the baseline we should be using: { 'u:up:SkmX6OL9M': 'manager' }. The current folder permissions don't matter.

On another test, where I didn't remove any users from the "Shared with" field, this is what I got:

resourceSubType:collabdoc
displayName:My public document
description:
visibility:private
managers:u:up:SkmX6OL9M
managers:u:uc:HyDW6O8qf 👈 
managers:u:up:SkmX6OL9M
folders:f:up:rkqGJYUqG

...which confirms my approach: this is what we need roles to contain. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I finally removed all my code (no code, no mistake haha) because I get all members on 3akai-ux and there is no need to add code in Hilary but there is something that still bothers me... On a collabdoc, if a user is added on the "setpermissions" modal, they will be added as manager.

-> So if there is 30 viewers on a folder, when a collabdoc is created inside the folder, this collabdoc will have 30 managers (+ the creator of the resource).

For me, a viewer of a folder should be editor(or viewer) but not manager ... I don't know, that's just my point of view.

(Just for information, viewers and managers of folder will become viewers of a link created inside the folder.)

Copy link
Member

Choose a reason for hiding this comment

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

For me, a viewer of a folder should be editor(or viewer) but not manager ... I don't know, that's just my point of view.

I agree. We need to make a folder viewer into a collabdoc editor.

@brecke brecke added this to the 15.1.0 milestone Apr 5, 2018
@ClaireLozano ClaireLozano force-pushed the document-in-folder-members branch from bfa596e to 5287a20 Compare April 9, 2018 14:40
@brecke
Copy link
Member

brecke commented Apr 11, 2018

:lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@brecke
Copy link
Member

brecke commented Apr 11, 2018

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


node_modules/oae-content/lib/api.js, line 543 at r1 (raw file):

Previously, brecke (Miguel Laginha) wrote…

For me, a viewer of a folder should be editor(or viewer) but not manager ... I don't know, that's just my point of view.

I agree. We need to make a folder viewer into a collabdoc editor.

This is the only comment from this review.. but I'm not sure this is done or whether this was just an idea, you tell me


Comments from Reviewable

@brecke
Copy link
Member

brecke commented Apr 12, 2018

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

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.

2 participants