-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Roles API + allow setting permissions via share attribues #36024
Conversation
50ac8bc
to
e0c449b
Compare
Codecov Report
@@ Coverage Diff @@
## master #36024 +/- ##
=======================================
Coverage 53.85% 53.85%
=======================================
Files 63 63
Lines 7377 7377
Branches 1301 1301
=======================================
Hits 3973 3973
Misses 3019 3019
Partials 385 385
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #36024 +/- ##
=======================================
Coverage 53.85% 53.85%
=======================================
Files 63 63
Lines 7377 7377
Branches 1301 1301
=======================================
Hits 3973 3973
Misses 3019 3019
Partials 385 385
Continue to review full report at Codecov.
|
fdc1924
to
d600986
Compare
@felix-schwarz @hosy @TheOneRing Could you have a look? |
d91b0b6
to
da2ee0a
Compare
The JSON and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See minor comments.
I'm missing unit tests for RolesController, at least to check that a few of the roles are returned (does not need to be 100% match).
Also need to test that the AddRoleEvent contents will actually appear in the roles API result.
} | ||
|
||
$pathinfo = \pathinfo($pathAttempt); | ||
$ext = (isset($pathinfo['extension'])) ? '.'.$pathinfo['extension'] : ''; | ||
$ext = isset($pathinfo['extension']) ? '.'.$pathinfo['extension'] : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems the automatic formatter did not decide to add spaces between the "." for concatenation (wenn schon denn schon!)
same for some other reformatting below
da2ee0a
to
48aa897
Compare
48aa897
to
2ef190a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
@DeepDiver1975, do you have a code example of how apps can listen to the event dispatcher and add their own roles? |
No app is currently using this - I suggest to leave this undocumented for the time being. I'll let you know as soon as we are at this point. THX |
Thanks for the clarification. |
Description
The roles api allows clients to ask the server for supported roles.
Currently only roles for public links are implemented.
Apps can listen to the event dispatcher and add their own roles.
The current concept does not reflect any capability for apps to change roles (core or from other apps)
Here is an example on how to submit permissions via the attributes property
Related Issue
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Open tasks: