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

[ClaireLozano]

Issue : oaeproject/Hilary#1407

---

This change is 

[brecke]

I think there was a misunderstanding.

There is no need to mess with visibility, I'm afraid. The radio button is already checked according to the visibility of the parent folder, all we're changing is the permissions.

Unless I'm missing something?

[ClaireLozano]

Oh yes you're right, oops :D
I changed that !

[brecke]

Is it ready then? to be reviewed again?

---

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[brecke]

A few things to fix, and one question to answer 👍

I haven't tested it locally, but will soon.

[brecke]

we could use _.chain here to make this simpler

[brecke]

why are you changing the order the names are being displayed?

[brecke]

instead of assigning a variable what is lost somewhere above, why not return it via callback? This way we avoid having "state" and we just pass arguments to functions accordingly.

[brecke]

Let's have this function returning the members as part of the callback, and then pass it as an argument to setUpAutoSuggest. Nice and easy.

[ClaireLozano]

It doesn't work when I do that :/ Should I let the code like this or should I push an unworking code to debug it together ?

[brecke]

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[brecke]

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

node_modules/oae-core/setpermissions/js/setpermissions.js, line 178 at r2 (raw file):

Previously, ClaireLozano (Claire Lozano) wrote…

It doesn't work when I do that :/ Should I let the code like this or should I push an unworking code to debug it together ?

There's a third option here, which is to push working code which is also elegant :)

So, for getFolderMembers to return the members, that function needs to return callback(null, member) somewhere. If you fetch the members, as an array, then you'll be able to do this, somewhere. And then you can have

getFolderMembers(pageContext.id, function(err, members) {´
if (err) {
...
}

// do something with members here

Does that make sense?

---

Comments from Reviewable

[brecke]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

node_modules/oae-core/setpermissions/js/setpermissions.js, line 143 at r2 (raw file):

Previously, brecke (Miguel Laginha) wrote…

instead of assigning a variable what is lost somewhere above, why not return it via callback? This way we avoid having "state" and we just pass arguments to functions accordingly.

This is still to be fixed. Let's use callbacks properly with callback(null, data);

---

Comments from Reviewable

[ClaireLozano]

Oops I pushed the code :P
It seems the code isn't executing the functions in the same order as in the previous commit ... or ... I don't know

[brecke]

Ok, so..

There's this: you forgot to declare folderMembers here but I think that somewhere you forgot to return a callback, because here:

oae.api.util.autoSuggest().setup($('#setpermissions-share', $rootel), {
                'allowEmail': true,
                'ghosts': widgetData.ghosts,
                'preFill': widgetData.preFill
            }, null, callback);

you're actually invoking setUpAutoSuggest(folderMembers, savePermissionsChange(folderMembers)); and that is not a callback, I'm afraid. At least I think that's where the problem lies just by looking at it.

[ClaireLozano]

I've made some changes, not sure it was the right thing to do but it works !
Ready to be reviewed I guess (?)

[brecke]

(but I still have to test it locally, when Hilary is ready to review...)

---

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[ClaireLozano]

(but I still have to test it locally, when Hilary is ready to review...)

But there is nothing to review on Hilary ... Maybe I've miss understanding you (?)
And why this comment doesn't appear on Reviewable ?

[brecke]

I didn't make that comment on reviewable :) sorry!

Regarding Hilary, I'll check that tomorrow to be sure

[ClaireLozano]

The redesign of the permission modal is done ;) ready to be reviewed !
There is only changes on the 3akai-ux project (forget the PR on Hilary)

[brecke]

Reviewed 33 of 33 files at r6.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @ClaireLozano)

---

node_modules/oae-core/createcollabdoc/js/createcollabdoc.js, line 116 at r6 (raw file):

                if (data.visibility) {
                    visibility = data.visibility;
                }

this could be more elegant: visibility = data.visibility || null;

Depending on what the elseis.

---

node_modules/oae-core/createcollabdoc/js/createcollabdoc.js, line 143 at r6 (raw file):

                        }
                    });
                }

If data.members and data.invitations are the same .length then perhaps we can just loop once and use index?

---

node_modules/oae-core/creatediscussion/js/creatediscussion.js, line 108 at r6 (raw file):

                if (data.visibility) {
                    visibility = data.visibility;
                }

Same here as before

---

node_modules/oae-core/creatediscussion/js/creatediscussion.js, line 130 at r6 (raw file):

                        }
                    });
                }

Same comment as before

---

node_modules/oae-core/createfolder/js/createfolder.js, line 109 at r6 (raw file):

                if (data.visibility) {
                    visibility = data.visibility;
                }

same here

---

node_modules/oae-core/createfolder/js/createfolder.js, line 127 at r6 (raw file):

                            managers.push(id);
                        } else {
                            members.push(id);

same here (the loops having the same size and all)

---

node_modules/oae-core/createlink/js/createlink.js, line 310 at r6 (raw file):

                if (data.visibility) {
                    visibility = data.visibility;
                }

same here

---

node_modules/oae-core/createlink/js/createlink.js, line 331 at r6 (raw file):

                            link.viewers.push(id);
                        }
                    });

same here

---

node_modules/oae-core/createlink/js/createlink.js, line 335 at r6 (raw file):

                    if (data.selectedFolderItems) {
                        link.folders = data.selectedFolderItems;
                    }

same notation here, to make it simpler: link.folders = data.selectedFolderItems || null;

is null what it's supposed to become if there is no data.selectedFolderItems?

---

node_modules/oae-core/setpermissions/js/setpermissions.js, line 120 at r6 (raw file):

            _.each(newMembers, function(newMember) {
                infinityScroll.prependItems(newMember);
            });

ok, so.. regarding newMembers we are currently doing a some underscore operations seperately, but we can easily make this a single chain command, right?

there's an each then a map then another each. I'm sure we can have this prettier instead of looping three times the same array.

---

node_modules/oae-core/setpermissions/js/setpermissions.js, line 159 at r6 (raw file):

            } else {
                $('#setpermissions-overview-visibility-container span').html(oae.api.i18n.translate('__MSG__PUBLIC__'));
            }

can you see the repetition here? all we're chaning is the html() argument so perhaps we can just define a variable to be set in the if statement and then call the html() just once after that. Makes sense?

---

node_modules/oae-core/setpermissions/js/setpermissions.js, line 202 at r6 (raw file):

                } else {
                    return callback(null, folderMembers);
                }

again, we can just return callback once and use the if statement to define the [] or the folderMembers

---

node_modules/oae-core/upload/js/upload.js, line 330 at r6 (raw file):

                if (data.visibility) {
                    visibility = data.visibility;
                }

same here

---

node_modules/oae-core/upload/js/upload.js, line 357 at r6 (raw file):

                    } else {
                        file.folders = [];
                    }

we can just file.folders = ...once, can't we?

---

node_modules/oae-jitsi-widgets/createmeeting/js/createmeeting.js, line 135 at r6 (raw file):

                if (data.visibility) {
                    visibility = data.visibility;
                }

same here

[ClaireLozano]

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @ClaireLozano and @brecke)

---

node_modules/oae-core/createcollabdoc/js/createcollabdoc.js, line 116 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

this could be more elegant: visibility = data.visibility || null;

Depending on what the elseis.

Hmm okay but it should be visibility = data.visibility || visibility; isn't that weird visibility = visibility ?

---

node_modules/oae-core/createcollabdoc/js/createcollabdoc.js, line 143 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

If data.members and data.invitations are the same .length then perhaps we can just loop once and use index?

it's not necessary the same length, data.member it's when you invite a member ant data.invitation it's when you invite a non-existing member by an email (when you send an invitation)

---

node_modules/oae-core/creatediscussion/js/creatediscussion.js, line 108 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

Same here as before

Done.

---

node_modules/oae-core/creatediscussion/js/creatediscussion.js, line 130 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

Same comment as before

Done.

---

node_modules/oae-core/createfolder/js/createfolder.js, line 109 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

same here

Done.

---

node_modules/oae-core/createfolder/js/createfolder.js, line 127 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

same here (the loops having the same size and all)

Done.

---

node_modules/oae-core/createlink/js/createlink.js, line 310 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

same here

Done.

---

node_modules/oae-core/createlink/js/createlink.js, line 331 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

same here

Done.

---

node_modules/oae-core/createlink/js/createlink.js, line 335 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

same notation here, to make it simpler: link.folders = data.selectedFolderItems || null;

is null what it's supposed to become if there is no data.selectedFolderItems?

Done.

---

node_modules/oae-core/setpermissions/js/setpermissions.js, line 120 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

ok, so.. regarding newMembers we are currently doing a some underscore operations seperately, but we can easily make this a single chain command, right?

there's an each then a map then another each. I'm sure we can have this prettier instead of looping three times the same array.

Done.

---

node_modules/oae-core/setpermissions/js/setpermissions.js, line 159 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

can you see the repetition here? all we're chaning is the html() argument so perhaps we can just define a variable to be set in the if statement and then call the html() just once after that. Makes sense?

Done.

---

node_modules/oae-core/setpermissions/js/setpermissions.js, line 202 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

again, we can just return callback once and use the if statement to define the [] or the folderMembers

Done.

---

node_modules/oae-core/upload/js/upload.js, line 330 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

same here

Done.

---

node_modules/oae-core/upload/js/upload.js, line 357 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

we can just file.folders = ...once, can't we?

Done.

---

node_modules/oae-jitsi-widgets/createmeeting/js/createmeeting.js, line 135 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

same here

Done.

[brecke]

Check the visibility = data.visibility || ALTERNATIVE_HERE thing in all the files

Reviewed 7 of 7 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brecke)

---

node_modules/oae-core/createcollabdoc/js/createcollabdoc.js, line 116 at r6 (raw file):

Previously, ClaireLozano (Claire Lozano) wrote…

Hmm okay but it should be visibility = data.visibility || visibility; isn't that weird visibility = visibility ?

Let's go through that:

visibility = data.visibility || visibility reads like the following: set visibilityto data.visibilityif that's set. If not, then set it to visibility. But that is the variable we want to set, so that's probably gonna throw an error. So we need null or something else for the alternative, right?

---

node_modules/oae-core/createcollabdoc/js/createcollabdoc.js, line 143 at r6 (raw file):

Previously, ClaireLozano (Claire Lozano) wrote…

it's not necessary the same length, data.member it's when you invite a member ant data.invitation it's when you invite a non-existing member by an email (when you send an invitation)

ok, forget about it then!

[ClaireLozano]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ClaireLozano)

---

node_modules/oae-core/createcollabdoc/js/createcollabdoc.js, line 116 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

Let's go through that:

visibility = data.visibility || visibility reads like the following: set visibilityto data.visibilityif that's set. If not, then set it to visibility. But that is the variable we want to set, so that's probably gonna throw an error. So we need null or something else for the alternative, right?

Hmm ... No (?) because visibility is already defined to null line 31 then line 104 we give it a value and then there is this line. So there should be no error (?)

[ClaireLozano]

Done.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brecke)

[brecke]

Reviewed 23 of 23 files at r8.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brecke)

[brecke]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brecke)

[brecke]

Reviewable status: complete! all files reviewed, all discussions resolved

---

node_modules/oae-core/createcollabdoc/js/createcollabdoc.js, line 116 at r6 (raw file):

Previously, ClaireLozano (Claire Lozano) wrote…

Hmm ... No (?) because visibility is already defined to null line 31 then line 104 we give it a value and then there is this line. So there should be no error (?)

ahh, you're right. Didn't see that line. Go ahead then.

[ClaireLozano]

Reviewable status: complete! all files reviewed, all discussions resolved

---

node_modules/oae-core/createcollabdoc/js/createcollabdoc.js, line 116 at r6 (raw file):

Previously, brecke (Miguel Laginha) wrote…

ahh, you're right. Didn't see that line. Go ahead then.

Done.

[brecke]

Reviewable status: complete! all files reviewed, all discussions resolved

[brecke]

Closed. See oaeproject/Hilary#1407