Propagation of Group Membership

Which closed issues are you referring to?

This is a feature request as the module does not currently provide this functionality.

Yes, that is incorrect.

If you have the following group structure:

California
-San Francisco

and post propagation is set to 'up the tree', then all posts inside of San Francisco will appear inside of California, and subscribers of California will, in effect, be subscribed to San Francisco as well.

This patch gives memberships the ability to propagate Up the tree. There are 3 new settings in th og_subgroups admin panel as well as 2 new settings when you are editing a PARENT group node if the admin propagate setting is set to group_controlled. I've commented this thing as much as I could. The maintainer should install it, check it for any weirdness and hopefully add it as a new feature. I use it on my own baseball league site and it works well so far.

This patch is for: // $Id: og_subgroups.module,v 1.26 2008/03/18 04:24:09 ezrag Exp $

NOTE: There is a bug in the og_galleries module that is tripped when you use this patch. I have a post about it here: http://drupal.org/node/122541#comment-823249 It's a bad error message on behalf of the galleries module. Simply commenting out the error message line will make the error go away. Whenever I modified the node edit form using hook_form_alter, og_galleries would start complaing about $vid being empty (even though it isnt). I'll have to look at the galleries module and submit a patch for that as well since that bug seems to have been around for over a year now. If anyone figures out why it does that, I'd be interested to know what the fix was.

This looks very interesting. I'm particularly interested in whether you can configure the system to promote all subgroup members, or only the admins of subgroups?

I look forward to reviewing this.

I haven't gone in depth over the patch but few things I have noticed:
1. To answer you comment from the patch, I think you are looking for hook_og, in order to tape into og actions. That will remove all the hackish code.
2. You have some coding style issues. Let coder module do its magic for you :).
3. I think the propagation should be up or down, according to the admins selection. Right now this patch, IMO caters only half of the use cases.
4. Documentation in doxygen format.
5. Might be out of the scope for this patch, but I think settings should be Per Member and Per admin.
6. function og_subgroups_user_operations, as you will get the user from hook_og you will know for sure the account which needs to propagate. Then I think it do the following:
..a. Get value of the up/ down tree settings.
..b. Check if user not already there/ not with a different role (i.e. member/ admin)
..c. Propagate.
7. function og_subgroups_parent_group_can_propagate - maybe it can return already the nids that should be propagated.

@Amitaibu - - Thanks for the review. I plan to review this weekend.

I agree with Amitaibu's point in #3 -- Propagation should happen in the same direction as post propagation. Maybe the simplest way to represent this in the admin interface is to add a single 'Propagate group membership' checkbox on the propagation settings area. I wonder if anyone would want to have membership propagate but not individual posts -- then we'd need an individual "propagation of group membership" checkbox for each possible propagation checkbox. More on this this weekend.

ezra-g said: "#3 -- Propagation should happen in the same direction as post propagation. "

I disagree, this should be user selectable as Amitaibu described, but could be different.

For example, imagine a parent group where only admins from subgroups propogate up as members, but those admins can then publish posts in the parent group that propogate down to all the subgroups. It could work both ways.

@scedwar: In this issue, could you describe a use case for that functionality?

My own use case of a team management system would certainly use it. I would have to consider the precise behaviour, but off the top of my head I can see the following structure of several parents for a group of (child) teams that would provide:

public parent (announcements group)
- admin members propogate up
- posts in the public parent are propogated down to all child groups

private parent (a 'resource' group)
- admin members propogate up,
- content is private and not propogated down

admin parent for moderation
- 2 or 3 admin members are propogated down to all subgroups to receive notifications
- all subgroups content is propogated up to the parent to provide a stream of subgroup (child) content

Unless it causes major coding problems, I don't think the propagation should be restricted to one direction. Content or members should be separately configurable to propogate up or down or both.

Stephen

2. You have some coding style issues. Let coder module do its magic for you :).

Thats not really a review, thats an opinion. I use Kate (KDE Editor) and I like the way it does things. I prefer my braces to actually be readable, not hidden behind some line of text. Theres nothing worse than missing a { or } in php because you'll end up with the $end of death error. ( ) are different, php gives good lines on those errors. But if you remove a brace, have fun figuring out where it is because PHP will give you the very last line of the file as the error line. So spare me the "coding standards" crap. I've been doing this long enough to know that my "style" suits me the best. I've had to hunt down my share of missing braces. So putting them on a line by themselves makes reading AND finding them 50000000000x easier than you could ever imagine. So, there you have it, thats why your line #2 is more of an opinion than anything relevant to the patch. The other lines look good to me. As I mentioned before the code I wrote was for my own use, it wasnt meant to be released. Ezra asked me in another thread to put up what I had so I told him I'd spend a day commenting everything and thats what I did. I commented all the changes, ran it through patch and there ya have it. It works, thats all that mattered to me personally. It could use some extra features but it has everything I needed for my own sites. If you could take the patch, make it better with more features, then I'm sure ezra would be cool with that too.

Lancelight: Thanks for submitting this patch. So I know what to expect moving forward with this feature, are you planning to not revise the patch further to, for example, incorporate propagation in two directions?

I'm not going to revise it any since I dont personally need a bidirectional feature myself but someone else probably will. The nature of opensource is that you contribute what works for you and in the end once everyone has contributed, you end up with a more featureful app than if someone tried to do all of it themselves all at once winblows. This project would be a good example of it. The dev didnt need that feature and thus someone else (me) coded it. So you could probably expect to see my patch either put in untouched or put in with lots of snazzy new features along with it or another patch against a patched version of subgroups lol. Confused yet? But that doesnt mean that this patch is incomplete at all. It works perfectly just the way it is albeit its got some quirks in it since I didnt know about the og_hooks stuff etc but it all works. The best thing about it is that someone who knows the OG module better than I do points out places where it could be streamlined. In a closed source app, you'd never have known that I had to use hackish code in 1 particular spot.

@Lancelight,
I appreciate your efforts, so thank you.
Regarding the coding styles this is certainly not an opinion. You will see core built this way, and no patch with coding errors ever gets commited. http://drupal.org/coding-standards

@ezra-g,
I will give it a stab as soon as I get time.

Yea I like the standards except for the whole { } thing. The problem is they base it off the pear project which in turn is based on some c/c++ standards which didnt take into consideration how much of a nightmare a missing {} can be in PHP as seen here by another who thinks like I do ;) http://pear.php.net/manual/en/standards.control.php. It's not like in C where it can tell you a meaningful line of about where the { } would be missing. Its also just more readable with the control brace being on a line of its own imho.

Sorry if I'm intruding, but I am also very interested in this bi-directional membership propagation option.

@Amitaibu,
I'm looking forward to see what you come up with.

I tried using the patch as provided, but it doesn't seem to work. A admin of a subgroup didnt not become a memember of the parent group. Just wondering if anyone else has tried this module out, or if there is something stupidly simple that I'm missing.

This is just an update from your friendly module maintainer. I was hoping to review this patch this past weekend but my schedule is extremely restricted until mid-May. I will try to do a more thorough review before then, but wanted to let everyone know that I haven't forgotten about this.

Thanks for everyone's interest in this issue!

Hi,

Perhaps i'm off subject, but in a hierarchy with multi level, and up propagation, it seems that posts just propagate in parent group and not in all parent groups. I'm looking for a way to make that but i don't find the function I need to change/make recursive.

Any idea someone ?

subscribing

I encourage you to check this issue - #255292: A complete rewrite of subgroups module and user/ content propagation.

Nice, i'll try this soon !

it works well (posts prapagate like i wanted) but the interface would deserve to be simplified

This is addressed in the 5.x-4.0 release, available in a few minutes.

@antipix: Feel free to open a new issue with suggestions for the user interface.

hmm...perhaps when i'll be more familiar with all that drupal stuff :)