Core has done that for example for the filter system.
Instead of custom tables, settings, UI and what not, we could simply expose 2 permissions per relationship type. This would make the UI simpler and save two queries for every page that uses relationships.
The only downside of this would be that it could result in a large list of permissions if you have many relationship types, but I assume that most sites will only have one or two. And this is already true for many other things like node types too.
| Comment | File | Size | Author |
|---|---|---|---|
| #28 | have_request_permissions5.patch | 39.95 KB | berdir |
| #25 | have_request_permissions4.patch | 37.42 KB | berdir |
| #21 | have_request_permissions3.patch | 34.36 KB | berdir |
| #18 | have_request_permissions2.patch | 26.58 KB | berdir |
| #17 | have_request_permissions.patch | 27.42 KB | berdir |
Comments
Comment #1
BenK commentedSubscribing... are you talking about the current "Allowed roles" tab on the individual relationship type?
--Ben
Comment #2
berdirExactly this.
As said above, core has moved all role/permission settings to the default permissions system instead of using custom checkboxes.
The permissions UI page *does* have a UI problem with many permissions, but that's not a problem we need to solve :)
Comment #3
BenK commentedOkay, sounds good. Yeah, I don't think it's a big deal about adding too many permissions to permissions page. The number of relationship types will be usually be a lot less than the number of content types.
Comment #4
berdirHere is a first patch. This will require some serious testing for the following reasons:
- With permissions, we always have to check them, not like before, where leaving the roles fieldset empty means everyone has access.
- If we have specific permissions for each type then, the generic 'can have relationships' permission has no meaning anymore and can be removed
- This mean that all calls which previously checked that permission need to be updated, but the new type is relationship type specific, so that needs some refactoring....
What needs to be tested:
- Users can only request relationships they are allowed to
- Users can only request relationships the other is allowed to have
- Users should only see the types they are allowed to have (user profile, relationship pages, request forms, blocks, ...)
Comment #5
YK85 commentedHi Berdir and BenK,
I was wondering if the work on 7.x in the issue queue will be eventually backported to 6.x as seen in Privatemsg module?
It would be really great if 6.x can be further developed and I would definitely be on board to help with testing.
Yours kindly
Comment #6
YK85 commented[removed duplicate]
Comment #7
berdirI don't have such plans right now, various new things depend on new D7 only features like the renderable arrays. We'll see...
Comment #8
YK85 commentedThank you for the quick reply!
I look forward to following the development of your modules!
Comment #9
BenK commentedJust bumping this so I remember to begin testing the patch...
Comment #10
BenK commentedHey Berdir,
Do you still want me to test the patch in #4 or do you want me to wait for this to be combined with our other efforts to revamp module permissions?
--Ben
Comment #11
berdirThe patch actually already works as discussed for the receive/have and request permissions. So go ahead and test this...
Comment #13
berdir#4: permissions.patch queued for re-testing.
Comment #15
BenK commentedBerdir,
I'm about to start testing, but do you want to address the simpletest fail first?
--Ben
Comment #16
berdirIt probably makes sense to fix the tests first.
Comment #17
berdirOk, updated patch.
- All tests are now working again for me.
- Also fixed an unrelated bug (privatemsg integration settings are not visible anymore) to get the tests working again
- Added an API function to help with access checks. This helps to merge different functions together and save some duplicated lines of code. It also addresses the use case that we want to check for a specific permission if a user has that permissions for at least one relationship type (for example menu items like "My relationships" should only be displayed if the user is allowed to view some relationship types)
- Removed the roles tables from the database. There is no migration of existing settings, I just added a message to inform users that they will have to do that. A lot is going to change with these permissions so they will have to reconfigure it anyway I think.
This should be ready for manual testing, even if the tests fail on the testbot (It's possible that something is messed up there). The current tests aren't that useful anyway, we only have some basic API tests and those from the Privatemsg integration (and they are currently not tested because of testbot bugs).
Edit:
For testing, there is a lot that needs to be tested here, I removed some access checks, for example in the blocks. Please note anything you think is related to this patch, we might postpone some stuff to other permission patches, if it's related to view permissions for example.
Comment #18
berdirJust a simple re-roll because the privatemsg admin settings issue was fixed in another issue.
Comment #19
BenK commentedI've started testing, but noticed a few major things that may need some work:
1. At the top of the relationship type edit page, I'm getting the following notice:
Notice: Undefined index: rtid in user_relationships_ui_form_user_relationships_admin_type_edit_alter() (line 700 of /Users/benkaplan/git/drupal-7.0b/sites/all/modules/user_relationships/user_relationships_ui/user_relationships_ui.module).
2. On the "friend" relationship type edit page, if I try to make a change and click save I get the following:
Notice: Undefined index: rtid in user_relationships_ui_form_user_relationships_admin_type_edit_alter() (line 700 of /Users/benkaplan/git/drupal-7.0b/sites/all/modules/user_relationships/user_relationships_ui/user_relationships_ui.module).
friend has already been used.
So when re-saving a relationship type, the system is acting like you're trying to create a new relationship type of the same name.
3. The new per relationship type permissions don't appear to be respected at all. All roles can create and request all relationship types even if they are not checked on the permissions page. On another user's profile page, I'm seeing all relationship types available, regardless of whether or not the user can receive those types or if I can request those types.
The only thing I didn't do is run update.php (so that I wouldn't have trouble reversing the patch). Could running the update somehow be required to use the new permission system?
--Ben
Comment #20
berdir1. I accidently deleted that part of the form. Fixed.
2. Same bug.
3. Yeah, as I said, this is not well-tested (As in: not at all :)) There were some serious bugs, which I fixed: In the can_request() and can_receive() API functions, I forgot to pass the current relationship type and user objects, so it always checked for all relationship types for the current user.
New stuff:
- I also did some refactoring in user_relationships_ui.module. Saves quite some duplicated code. Shouldn't affect you directy, but make sure that the request relationship links on the user account (and request form when direct links are disabled) are correct. Right now the request form doesn't respect the permissions and "allow multiple" settings correctly I think. I also plan to move some more functions to user_relationships that really belong there.
- Made sure that the "My relationships" menu doesn't show up if you have no have relationship permissions.
Comment #21
berdirHit Save too fast.
Here is the patch.
Comment #23
berdir#21: have_request_permissions3.patch queued for re-testing.
Comment #25
berdirOk, fixed some bugs in the revamp listings patch and fixed the tests.
Comment #26
BenK commentedHey Berdir,
The patch is working very well... Previously reported issues 1, 2, and 3 are now solved. And permissions are being respected throughout.
As for your new stuff:
* I checked that relationship request links are all working (both direct and via the form). It's working great. Actually, in my testing it seemed to be working better than you thought. When I use the form, I'm getting permissions respected and the multiple relationships setting is working. So it seems great. Are you sure that those features are not supposed to work? ;-)
* I confirmed that My Relationships menu doesn't show up if you don't have the ability to accept any relationships.
Here's the only issue I'm experiencing....
4. When saving a change to an existing relationship type or creating a new relationship type, I'm getting the following warning:
Notice: Undefined index: roles in user_relationships_admin_type_edit_submit() (line 365 of /Users/benkaplan/git/drupal-7.0b/sites/all/modules/user_relationships/user_relationships.admin.inc).
Notice: Undefined index: roles_receive in user_relationships_admin_type_edit_submit() (line 380 of /Users/benkaplan/git/drupal-7.0b/sites/all/modules/user_relationships/user_relationships.admin.inc).
5. What other functions do you plan on moving to user_relationships?
--Ben
Comment #27
berdir* It is actually supposed to work, but I think it doesn't work correctly in the current version of UR - That is what I meant.
4. Will look at that soon.
5. Probably at least the function that returns which relationship types are possible between two users. That's not UI specific, so it doesn't belong there. Maybe more, but that's something that can be done later.
Comment #28
berdirOk, fixed that error and also one that occured when deleting a relationship type.
Comment #29
BenK commentedI've confirmed that the warnings are now gone in the latest patch. So except for moving that function to user_relationships, this is RTBC.
I'll hold off on marking it as RTBC, however, until we can move any functions that you feel would help clean up the code.
--Ben
Comment #30
berdirThen it is RTBC.
I wasn't clear above, I want to do the function movingin a new issue. That was just a reminder for me.
Comment #31
berdirCommited, will open a new issue for the function moving stuff.