As discussed with Berdir in IRC, we're planning on implementing new permissions per relationship type for UR core and UI permissions. The following permissions (for each relationship type) will be implemented on this thread:
Core (user_relationships.module):
a) "Approve or decline [relationshipname] relationship request"
b) "Remove own [relationshipname] relationships"
UI:
c) "View own [relationshipname] relationships"
d) "View all [relationshipname] relationships"
Did I leave anything out?
--Ben
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 1115998_another_reroll.patch | 33.49 KB | berdir |
| #13 | access_checks_for_blocks.patch | 32.97 KB | berdir |
| #12 | new_permissions4.patch | 32.74 KB | berdir |
| #8 | new_permissions3.patch | 31.72 KB | berdir |
| #6 | new_permissions2.patch | 28.83 KB | berdir |
Comments
Comment #1
berdira) and b) will replace maintain, do I remember that correctly?
Comment #2
berdirJust re-read the meta issue, yes it does ;).
Comment #3
BenK commentedYup, that's right. :-)
Comment #4
berdirOk, this is quite a complicated patch. A few important hints:
- The mailer and privatemsg modules currently contain a permission check for the maintain permission for their user settings. That can't be replaced with either approve or delete. I don't want to add another permission just for that, so I'm wondering if we want to keep naming the approve permission maintain and simply describe what it means in the description
- I noticed an issue with the permissions checks for local tasks which currently completely messes up the access checks (access is only checked for the currently active tab). This is a core bug and I have a patch that fixes this, will post a link as soon as I created an issue for that.
- It will be a lot of work to correctly check that all permissions work together. Make sure that always only the UI elements show up that make sense (request links, approve links, and so on)
- I have not yet touched the blocks module.
Comment #5
berdirIssue for the core bug #1137052: Argument map of parent router item is used when checking access for local tasks.
Comment #6
berdirRe-rolled the patch, should apply again.
Comment #7
BenK commentedHey Berdir,
Thanks for the patch... here are my notes.
a. I have a few suggestions in terms of permission names. Here's the four I think we should standardize on. In this example, I'm using the "friend" relationship type:
* Have friend relationships
* Maintain friend relationships
* Request friend relationships
* Delete friend relationships
I've tried to get each of the above to only be three words and in the identical format. I think this makes it easier to scan.
b. Can we change the order of permissions so that they appear in the order shown above. I think it's a bit more logical to have "maintain" before "request" in the list of permissions (so that they appear from least permissive to most). And yes, I agree we can use "maintain" instead of "approve" for the reasons you outlined.
c. Is there any way to make the relationship type name italic in each permission on the permissions page. I'm not sure if this is possible, but it would be nice to have "friend" be italic so that the permissions are easier to scan.
d. In actually testing the permissions, I've noticed that only those with the "Administer User Relationships" permission are actually seeing the "My relationships" menu link and the "Relationships" tab on each user. As a result, if a non-admin user receives a relationship request and tries to follow the "you have pending relationship requests" link, the user receives an "Access denied" error. Once this is fixed I can do more testing. (But I can say that the permissions seems to function properly for the admin user.)
Thoughts?
--Ben
Comment #8
berdira) Renamed.
b) Changed. Although it's arguable what's more or less permissive :)
c) Yes we can! ;)
d) Uhm, dumb question maybe, but since you haven't mentioned them yet.... Are you sure that you have set up the View own/all permissions? Since they've been renamed, you need to re-configure them, just like the others. Because it works for me (tm).
Comment #9
BenK commentedThe latest patch is looking and working great. All previously reported issues have been resolved. I just noticed the following things:
1. Should we standard all of the permission groups to use "User Relationships" rather than "UI"? Currently, because the "UR -UI" group and the "User Relationships" group have the "User" permissions group between them, it's easy to forget one group of permissions. I know that we previously tried to make things consistent around "UI", but now might be the time to standardize around "User Relationships" instead. So for example, here would be the new permission groups:
* User Relationships
* User Relationships UI
* User Relationships Private Message
* User Relationships Node Access
2. Do we need to apply the new permissions to the various blocks supplied by UR?
3. When a user doesn't have permission to do any operations on a relationship type, then the operation cell in the user's relationship list doesn't seem to have any background color. I'm OK with this, but just wanted to bring it up. Not only does "Remove" not display, but the entire table cell is missing the color.
4. I'd change the permission descriptions to read:
a) Have:
"The user may have relationships of this type."
b) Maintain:
"The user may approve or decline relationship requests of this type."
c) Request:
"The user may request relationships of this type."
d) Delete:
"The user may delete current relationships of this type."
5. Also, on the view all/own permission descriptions, I'd change the phrase "of that type" to "of this type".
--Ben
Comment #10
berdir1. These names are simply the name of the modules. I fully agree that we should rename it, but we should rename them all (not simply those that define permissions) and in a follow-up issue. Is there maybe even one open already?
Comment #11
amandawolfe commentedTried rolling the most recent patch with the latest commit of 7.x-1.x-dev and I'm getting this patch error:
Hunk #18 FAILED at 444.
1 out of 20 hunks FAILED -- saving rejects to file user_relationships_ui/user_relationships_ui.module.rej
I'm not super-experience with patching so I'm afraid I can't really offer any help, but I wanted to let you know. Thanks for all your work on this module!
Comment #12
berdir1. Separate issue, as explained in #10.
2. We do, but let's make a separate issue as well to continue with this stuff.
3. Should be fixed now.
4. Renamed.
#11: Re-rolled the patch, this should apply against the latest -dev version again. Oh, and thanks for reminding me of this, let's get this back on track! Any feedback would be very welcome.
Comment #13
berdirThe attached patch also implements permission checks for the blocks, please test.
Comment #14
berdirHere is an update that should make this work again with the newly added tests.
I'm tempted to go ahead with this and commit it, so that we're able to continue...
Comment #15
berdirTriggering test bot...
Comment #16
berdirOk, I've commited this and also some new tests that I wrote.