The OG: Approve/Deny/Remove membership link handler adds the og_uid.nid additional field to Views queries but doesn't check to see whether there is a WHERE clause on this field. This means that the rendered link may have rather unpredictable results as to which group membership you are editing when the field is used without a group argument/filter.
Two possible options seem reasonable to me:
- Check the $view->query object for a WHERE clause against og_uid.nid. While at first glance more accurate, it can become a very convoluted check as you need to ensure the WHERE will filter against only one nid -- multiple nids will again create confusion.
- Check og_get_group_context() against the nid returned by the Views query. This appears to cover most sensible use cases, as the Views OG argument handler will set the group context anyway.
The attached patch takes the 2nd route. Applies against DRUPAL-6--2 and 2.0 stable as well.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | og_views_og_uid.patch | 448 bytes | jcmarco |
| #11 | og_views_og_uid.patch | 795 bytes | jcmarco |
| #9 | 701420_2010-02-28.patch | 16.6 KB | yhahn |
| #3 | 701420_2010-02-23.patch | 3.88 KB | jmiccolis |
| og_views_managelink_groupcontext.patch | 1.28 KB | yhahn |
Comments
Comment #1
marcp commentedThis is just a quick code review -- I haven't digested the issue at all.
This line in the patch:
shows parentheses around the === comparison but no parentheses around the != comparison. It doesn't really matter based on the operator precedence rules, but it would be nice if this was consistent (and even nicer if it included parentheses since that makes the code a little bit clearer.
just seems a little bit clearer.
Comment #2
yhahn commentedThanks for the quick style review - once I get some indication that the substance of the patch is going in the right direction I'll reroll with the change.
Comment #3
jmiccolis commentedAttached is and version of the same patch that makes the change recommended in #1 and adds this check to the 'manage link admin' and 'og is admin' field handlers.
Comment #4
amitaibuI'd like to better understand -- what is the View you are using and what are the unpredictable results you get?
Also, the OG argument handler sets the group context or the first node ID, so with the provided patch I think that in case of a list of groups, only the first group will have a link, but the others won't
Comment #5
yhahn commentedThe basic use case here is a view which can be used both inside and outside of a group context. In particular we have a view (base table:
users) that lists users. When a group context is active the view works fine but outside of a group context and/or without a group arg it doesn't make sense to show the OG management links show at all (they show without the patch, and don't make any kind of sensible action).Comment #6
amitaibuWhat I worry with this patch, is if a user has a view with node base table of all the groups, and wants to show the manage link next to them.
Maybe -- #711354: Add a "current group" default argument handler can solve your use case?
Comment #7
yhahn commentedThat's a good point -- what do you think of only executing this check if the View is using the users base table?
In one sense this problem exists because
og_uidcan be joined to from either the node base table or the users base table. From the node table the field handlers are working against the node nid of the row selected which is fine. But when using the users table unless the View has explicitly limited the set of groups (either by group context or explicit argument) the handlers are just not going to generate anything sensible. Note that this affects nearly all theog_uidfields -- on a view of users outside a specific group context, "Edit membership", "Is active", etc. all don't make any sense. Edit membership to what? Is active in what?If this approach seems reasonable I will reroll the patch.
Comment #8
amitaibu> what do you think of only executing this check if the View is using the users base table?
I think this is a better approach, because as you say -- if it's the users base table the View is probably "show me users from group foo".
Comment #9
yhahn commentedHere is the rerolled patch. It's quite a bit larger, partly because it cleans up a lot of the logic in the
og_uidhandlers.og_views_handler_field_og_uidbase field class which encapsulates the logic above at thequery()method level. This means that when using base tableusersand not in a group context the field doesn't create a join toog_uidat all since it won't be displayed anyway. Turns out to speed up quite a few queries.og_uidfields now extend this base class. The Views data provided for these fields have been adjusted and many of them take advantage of thereal fieldflag that Views provides instead of the variety of hacks previously used in the handlers.I've tested these handlers against
usersandnodebase tables under the scenarios described above (inside group context, outside group context, as a list of groups for a given user, etc.) It would be great to get some more testing.Comment #10
amitaibuGood appracoh, thanks! committed.
Comment #11
jcmarco commentedThe patch was applied incompleted.
The file og_views_handler_field_og_uid.inc is lost,
so any field and filter using og_uid is not working.
Comment #12
amitaibu@jcmarco,
I'm not sure I understand your patch:
1) og_views_handler_field_og_uid.inc was committed
2) In your patch the class is
og_views_handler_field_og_is_admin-- which is also there - og_views_handler_field_og_is_admin.incPlease explain ;)
Comment #13
jcmarco commented@amitaibu
If you check the og-6.x-2.x-dev.tar.gz. you will check that file og_views_handler_field_og_uid.inc doesn't exist.
In any case I made a mistake creating the patch, I used a different file, that is because my patch is so weird, sorry about that.
This is the new patch with the lost file from @yhahn patch
Comment #14
amitaibuWeird, I remember adding this file -- thanks for the follow up.
Comment #16
DanielJohnston commentedFor reference, I've posted a follow-on to this issue at #883516: og_get_group_context() should pick up on relationships as well as direct arguments, as the solution here appears to be zapping my attempts at creating a view on users that takes the current user's admin gid as its context. Thought a new issue was better than reopening this as it's been closed for a while.