Skip to content

Fix share permission checkboxes enabled when permissions can not be set #21379

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
Merged
merged 5 commits into from
Jun 24, 2020
Merged

Fix share permission checkboxes enabled when permissions can not be set #21379

merged 5 commits into from
Jun 24, 2020

Conversation

@danxuliu
Copy link
Member

Note that the acceptance tests will probably fail because the Files app is currently broken due to the jQuery update, but I run them locally with jQuery 2.1.4 and they passed.

How to test (scenario 1)

  • As user A, share a file with user B
  • Open the menu for the share and uncheck "Can edit"
  • As user B, reshare the file with user C
  • Open the menu for the share

Result with this pull request

Can edit is unchecked and disabled.

Result without this pull request

Can edit is unchecked and enabled. Trying to check it triggers an error.

How to test (scenario 2)

  • As user A, share a file with user B
  • As user B, reshare the file with user C
  • As user A, open the menu for the share and uncheck "Can edit"
  • As user B, reload the page and open the menu for the share
  • Can edit is checked and enabled
  • Uncheck Can edit

Result with this pull request

Can edit is unchecked and disabled.

Result without this pull request

Can edit is unchecked, but still enabled. Trying to check it triggers an error.

danxuliu added 5 commits June 11, 2020 23:12
@danxuliu
Generalize functions to get locators for share permission checkboxes
cafa684 
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Add acceptance test for sharing a file without edit permission
573bf4d 
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Add acceptance test for sharing a folder without create permission
33222d6 
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Add acceptance test for disabling create permission after sharing
2c6d506 
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Fix share permission checkboxes enabled when permissions can not be set
3ffb482 
A sharee can reshare a file and set the edit, create, delete and share
permissions of the reshare only if the received share has edit, create,
delete and share permissions, or if they were revoked in the received
share after being set in the reshare. Therefore, the permission
checkboxes in the share menu should be enabled only if the user can set
them (otherwise trying to check them will lead to an error).

Note that "sharePermissions" has all the permissions if the file is not
a reshare but a file owned by the user.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu added bug 3. to review Waiting for reviews labels Jun 11, 2020
@danxuliu danxuliu added this to the Nextcloud 20 milestone Jun 11, 2020
@danxuliu
Copy link
Member Author

/backport to stable19

@danxuliu
Copy link
Member Author

/backport to stable18

rullzer
rullzer approved these changes Jun 15, 2020
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

🐘
but with so many tests... 🚀

@ChristophWurst
Copy link
Member

Note that the acceptance tests will probably fail because the Files app is currently broken due to the jQuery update, but I run them locally with jQuery 2.1.4 and they passed.

Mind elaborating? Is the the one issue where the jQuery load event fires too soon, like before some of the script have been loaded?

@danxuliu
Copy link
Member Author

Note that the acceptance tests will probably fail because the Files app is currently broken due to the jQuery update, but I run them locally with jQuery 2.1.4 and they passed.

Mind elaborating? Is the the one issue where the jQuery load event fires too soon, like before some of the script have been loaded?

I am afraid that I do not know if the issue is the load event firing too soon or something else. It could very well be, though; the problem is that, sometimes, the file list does not load in the Files app if jQuery 2.2.4 is used (I recall seeing an error in the console about OCA.Files.Sidebar.close() (or something similar) not being defined), but it works fine if jQuery 2.1.4 is used.

@ChristophWurst
Copy link
Member

I your observation matches with my problem because of the way our legacy modules register themselves on the global variables. If one of the modules is not loaded when I dependent of it is executed, you'll get an error.

@rullzer
Copy link
Member

rullzer commented Jun 24, 2020

So... to merge or not to merge?

@danxuliu
Copy link
Member Author

So... to merge or not to merge?

Merge, we were talking about why the acceptance tests for the Files app fail since the jQuery bump, but it does not affect the fix itself :-)

@rullzer rullzer merged commit 7d4682d into master Jun 24, 2020
@rullzer rullzer deleted the fix-share-permission-checkboxes-enabled-when-permissions-can-not-be-set branch June 24, 2020 19:20
This was referenced Jun 24, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rullzer rullzer rullzer approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@GretaD GretaD Awaiting requested review from GretaD

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees

No one assigned

Labels

3. to review Waiting for reviews bug

Projects

None yet

Milestone

Nextcloud 20

Development

Successfully merging this pull request may close these issues.

4 participants

@danxuliu @ChristophWurst @rullzer