Hierarchical groups

[sooskriszta]

http://buddypress.org/community/groups/requests-feedback/forum/topic/need-for-hierarchical-groups/

​http://wordpress.org/support/topic/plugin-buddypress-hierarchical-groups

Hierarchical groups functionality should be part of the core, complete with member propagation – giving 3 options to admin … upwards propagation, downwards propagation, no propagation…

Not having hierarchical groups in BuddyPress core is like if hierarchical pages were not part of WordPress core…

[boonebgorges]

Not having hierarchical groups in BuddyPress core is like if hierarchical pages were not part of WordPress core…

That's a bit extreme. Hierarchical groups would be useful for some implementations of BuddyPress, but it's not obvious that it's overwhelmingly required for most instances.

From the data schema point of view, hierarchical easy - just allow for a parent_id or something like that. But there are a ton of questions to be asked about what this means for users. Member propagation is one question, as you've addressed above. But what else would the parent-child relationship mean for groups? Would aspects of the groups be merged (forums, activity, etc)? Would each group have a display of its child and parent groups, perhaps in the header? Would this require additional filters on group directories, or possible tree-like views of group directories?

Again, the technical issues here are (mostly) not that difficult. But it's only worth adding the feature(s) if it can be done in a way that is clear to all users, and that proves useful to most.

In any case, I have a couple of alternate suggestions to bundling all of this with BP:

1) Build proper group taxonomy and meta support. If BP had built-in support for group categories and/or tags, it would solve a big part of the impetus behind group hierarchy (that is, explaining that certain groups are related to each other). IMO, group taxonomy is of much broader appeal than standalone hierarchy, and it's clearer (to me at least) how it would be implemented from a UX point of view. If this were done, then it would be pretty easy to build a plugin that does the additional work of, eg, propagating group membership among similarly categorized groups.

2) Build better support for ddean's BP Group Hierarchy plugin ​http://wordpress.org/extend/plugins/bp-group-hierarchy/ into BP core (ddean - I added you as a CC to this ticket - hope that's not too rude :) ). This plugin is popular and highly regarded, but I think that David has run into a couple of problems with implementation that have led to some less-than-ideal situations. For one, his plugin has to modify BP's groups table to add a parent_id column; we might consider doing this in BP itself. He's also built his own version of bp_has_groups() just so that he can add hierarchy support, and he does some clever tricks to mess with BP_Groups_Template. If we add a parent_id column to our db schema, we could also add hierarchy support (parent_id, and maybe others if it's relevant) to the bp_has_groups() stack of functions. Then we let the plugin provide all the necessary UI elements, like it already does.

In the short term, I lean toward the latter. It's something we can do fairly easily, it'll make ddean's life much easier, and it won't affect BP users who aren't interested in group hierarchy. Long term, we could then talk as a team about how/whether to implement some of the user-facing stuff in BP in future versions.

[DJPaul]

I think adding a database column and modifying groups' query stack for something we aren't going to expose directly in core is not the right approach. If we came to the decision that this is something we wanted to do in core, we can certainly build these parts in, even if the release doesn't allow us enough time to build a UI for it (leaving it to the plugin).

[ddean]

Hi, all! Sorry for being tardy; I didn't have an email address set in my Trac profile, so I didn't get the cc.

I thought the long term plan was to make groups into a custom post type, with the built-in taxonomy support that provides, so I've been happy to keep patching things in until then. Is that still the ultimate goal?

While I appreciate anything that makes my life easier :), most of the integration work has already been done and, for compatibility's sake, couldn't just be taken out. Whatever you all decide to do is fine, and I'd be happy to provide a short list of changes that could improve extensibility.

This seems to be the key:

Again, the technical issues here are (mostly) not that difficult. But it's only worth adding the feature(s) if it can be done in a way that is clear to all users, and that proves useful to most.

If download counts are to be believed, hierarchical group users account for ~1% of BuddyPress users, and I've received only a handful of requests for membership propagation. And, though I can certainly see the value of this feature, the time needed to build and test a UI that makes it flexible enough to be useful without becoming a management or security issue has been enough to keep me away from it so far.

[sooskriszta]

Innovator's dilemma. Should we go after current audience or a bigger market out there.

Does use of hierarchical groups by 1% of BuddyPress users mean it is not a much-needed function or does it mean that the plugin doesn't do what users want it to do? (no offense ddean..i don't mean it doesn't do what it's intended to do...subtle difference)

I remember the days when WordPress was simply a *blogging* platform and didn't have hierarchical pages. Look at it now - since this became a core feature, a good proportion of people use WP as their CMS, including yours truly.

I truly believe that hierarchical groups with upwards/downwards propagation will open BuddyPress up for users and uses that are beyond its reach at the moment...

[boonebgorges]

I thought the long term plan was to make groups into a custom post type, with the built-in taxonomy support that provides, so I've been happy to keep patching things in until then. Is that still the ultimate goal?

Maybe. We have to clear some conceptual hurdles first, related to scaling and schema. IMO, the two easiest components to map onto CPTs are Messages and Groups (though not group members). I may write a proof-of-concept after 1.6 comes out, but it will be a while before those decisions become final.

Regardless of the CPT situation, I think that we should move forward with proper meta/tax for groups. On that note, see ​https://buddypress.trac.wordpress.org/ticket/4017.

[sooskriszta]

I don’t suppose a ton of people currently use hierarchical groups in BuddyPress. But I am certain that features like this will go a long way in opening up new markets for the script, way beyond just small techie groups.

It’s a usability thing. Let me take an example:

Site for networking of dentists in the UK.
What I have in mind is at top level 4 groups:

• England
• Wales
• Scotland
• Ireland

Then within each group, there are subgroups for counties.

Within each county subgroup, there are sub-subgroups for town/city etc.

Within the county sub-subgroup for town/city for particularly large cities, there could be sub-sub-subgroups for boroughs.

If Paul joins the group for Exeter, he should automatically become a member of Devon and England respectively.

I’m sure there might possibly be other ways of doing this (e.g. adding categories or tags); I just don’t see anything so instinctive or natural.

There can be multiple other examples.
For a medical professionals organization:
Doctor > Neurology > Clinical neurophysiology
For a university:
Business School > MBA > Managerial Accounting
etc

[sooskriszta]

24000 downloads of @ddean's Group Hierarchy plugin ​http://wordpress.org/plugins/bp-group-hierarchy/stats/ which is about 1.6% of total BuddyPress downloads. Consider also that BuddyPress has been out since April 30, 2009 while the plugin was first released in February 2011 and the actual proportion of the plugin users is likely to be much higher perhaps as high as 3-4%.

Further, 100% of the active installations of BP Group Hierarchy are of the latest version, meaning that these folks are likely using the latest version of BuddyPress too. If that's the case, and if we can use downloads as a proxy for active installations (and it may not be):
BP Group Hierarchy 1.3.8 active installs ~ 25,000
BuddyPress 1.7+ active installs = 1,562,730 x 32.6% ~ 500,000
BP Group Hierarchy 1.3.8 active installs as a proportion of BuddyPress 1.7+ active installs = 5%

This is still a small minority and I'd be the first to admit that for quite a while hierarchical group use will remain a minority activity, though it will probably be a bigger minority.

What I do posit is that

1. it is not an insignificant minority.
2. the function will help grow the pie.

[sooskriszta]

2.0? :-)

[vernonfowler]

30,510 downloads of BP Group Hierarchy to date 31/3/2014
591 downloads of BP Group Hierarchy Propagate - ​http://wordpress.org/plugins/bp-group-hierarchy-propagate/
I love both these plugins.

As much as some of our communities depend on these plugins layered on top of BuddyPress, I can't see Group Hierarchy making it into core for as long as it has less than 50% of the number of BuddyPress installs.

I'd rather find ways to further optimise BuddyPress itself (and bbPress for that matter). Right now P3 (Plugin Performance Profiler) reports 24% (0.3533 secs) and 12% (0.1749 secs) of my load times are dedicated to these 2 plugins respectively - the 2 heaviest plugins I have (shortly followed by Adminimize at 11%).

[sooskriszta]

One big unresolved issue is propagation:

If Group 1 is a parent, and Groups 2 and 3 are children of Group 1, then should someone joining Group 2 automatically become a member of Group 1? Or should someone joining Group 1 automatically become a member of Group 2?

Theoretically, both approaches seem quite valid. However, let's look at live cases and why folks want hierarchical groups. The parent-child relationship essentially a bit like successive filters applied to the groups. At the top level (say Group 1), there's everyone. Then we filter down by interest/ specialization/ location/ class/ what have you. So in Group 2 we have fewer folks - only those that are into Group 2 stuff. Then we have Group 4 - a child of Group 2. It has even fewer folks. Super specialists.

In other words, in live situations, it seems that the dominant use case is one where someone (say A) joining a child should automatically be added to the parent. (of course, there needs to be a check so that if A is already a member of 1, then that doesn't throw an error when A joins 2).

Also, there would be some communities that would not want propagation and would want parents and children to operate independently. Thus, propagation should be a checkbox option.

What of activity streams? Same upwards propagation principle applies. As anyone who has used a CRM software can attest, if I post something in the activity stream of a contact person then that propagates up to the activity stream of the contact person's organization's activity stream.

[sooskriszta]

So, for instance:
University A uses BuddyPress for its Intranet.
Top level group is Professors.
2nd level groups are Schools: Graduate, Law, Business and Medicine.
Subgroups are departments, e.g. at Business School: Accounting, Finance, Marketing, Operations and Organizational Behavior.

The University would want everyone joining Finance group to also be a part of the Business School group and Professors group.

It may switch activity propagation off or on. If it turns activity propagation on then the discussions at Finance group percolate all the way up to the Professors group. If activity propagation is off then they don't.

But if there is an announcement that it needs everyone to read, the university only wants to post it once on Professors group and wants everyone to read. Well, hey, there is no need for the downwards propagation of activity stream - all professors are already propagated up and can read the announcements posted in the Professors group!

[sooskriszta]

Would #4017 bring us closer to hierarchical groups?

[boonebgorges]

Would #4017 bring us closer to hierarchical groups?

Not strictly speaking, though you could definitely use group taxonomies to fake the parent/child relationship. (A truly general solution would be for WP to adopt a relationships API. See ​https://core.trac.wordpress.org/ticket/14513.)

[abweb]

Archive contains patch file and required files

[dcavins]

I intend to explore adding the basics of hierarchical groups for 2.7. I will not be addressing any sort of member propagation, only relationships and the groups query (the core bits).

[dcavins]

I'm attaching a first patch for adding hierarchical groups in BuddyPress. This includes the following changes:

• New parent_id column to the groups table in the database.
• Setting for enabling hierarchical groups.
• Per-group setting for parent group.
• Per-group setting for what kind of member can create subgroups of the group.
• Functions for finding the parent ID, ancestor group IDs and descendent groups.
• A bp_user_can() filter for whether a user can create subgroups of a particular group.
• New screen to show subgroups of a group at /group-name/hierarchy.
• Breadcrumb template function, bp_group_permalink_breadcrumbs().
• Tests.

What this patch does not do:

• Change the group directory to somehow display hierarchical relationships.
• Change the url for a group to be /groups/parent-group/this-group/
• Any kind of membership propagation.

Other notes:

• bp_groups_get_child_groups(), bp_groups_get_child_group_ids(), and bp_groups_has_children() would all be unnecessary if the group visibility pieces from #6677 were added to BP. Then, you could just use groups_get_groups() or bp_has_groups() with dynamic group visibility, now that parent_id support is added in BP_Groups_Template.

Please test and give feedback. I'll also attach screenshots of some of the controls below.

Thanks in advance for your thoughts and criticism.

[dcavins]

First patch

[dcavins]

Main group hierarchy setting in BP options screen.

[dcavins]

Single group hierarchy options screen, front end.

[dcavins]

Single group hierarchy options screen, wp-admin.

[dcavins]

New subgroup screen for a group.

[mercime]

@dcavins Wow! That's :alot: of work there :) Thank you.

Took this out for a spin a few minutes ago in a regular WP installation :)

• Re Parent Group select in wp-admin and front-end: only 20 groups show up in the select box out of the 76 expected (77 groups less the group you're editing).
• Suggest removal of the "Member Groups" link from a group's navigation if it is not a Parent Group.
• Add missing explicit label for Parent Group's select element in front end and back end.
• Re "Member Groups" - I can imagine that those who have been using the Group Hierarchy plugin would already know what "Member Groups" refers to. The terminology could be confusing to new users who could think that it's a list of groups where the members of the specific group are also members of.

[dcavins]

Replying to mercime:
Thanks for the test drive! Comments on your comments below.

@dcavins Wow! That's :alot: of work there :) Thank you.

Took this out for a spin a few minutes ago in a regular WP installation :)

• Re Parent Group select in wp-admin and front-end: only 20 groups show up in the select box out of the 76 expected (77 groups less the group you're editing).

Ha! Pagination! I'd really like to improve on the select anyway by making it an AJAX autocomplete type thingy. My main site has 600ish groups, so a select is pretty terrible.

• Suggest removal of the "Member Groups" link from a group's navigation if it is not a Parent Group.

I've followed BPGH's logic here: Show the tab if subgroups exist or if the current user can create subgroups (BPGH includes a "create" link on that screen). It seems a bit subtle for me, but BPGH also makes it possible to restrict who can create top-level groups and who can create subgroups, so I guess it's useful in that case where the current user can't access the regular "create group" button, but could create a subgroup. Do we want that kind of granularity?

• Add missing explicit label for Parent Group's select element in front end and back end.

Great!

• Re "Member Groups" - I can imagine that those who have been using the Group Hierarchy plugin would be already know what "Member Groups" refers to. The terminology could be confusing to new users who could think that it's a list of groups where the members of the specific group are also members of.

I too am conflicted. What should these objects be called?

Thanks very much for your comments! I'll work on a new iteration of this next week.

I'm still trying to figure out how I'd like to reflect hierarchy on the main groups directory--like BPGH with subgroup expands, simply including breadcrumbs with each result, or something else? The AJAX expand that BPGH uses seems to cause issues for users who expect to scan the list and find the group they're looking for, and never find second or deeper level groups because they don't show up. Maybe show them more like threaded comments with the option of closing groups of groups (could be a pretty expensive page load, and pagination would be odd)? Opinions welcome!

[sooskriszta]

Replying to dcavins:

Replying to mercime:

• what "Member Groups" refers to. The terminology could be confusing to new users

I too am conflicted. What should these objects be called?

Subgroups?

[sooskriszta]

Replying to dcavins:

I'm still trying to figure out how I'd like to reflect hierarchy on the main groups directory--like BPGH with subgroup expands, simply including breadcrumbs with each result, or something else? The AJAX expand that BPGH uses seems to cause issues for users who expect to scan the list and find the group they're looking for, and never find second or deeper level groups because they don't show up. Maybe show them more like threaded comments with the option of closing groups of groups (could be a pretty expensive page load, and pagination would be odd)?

How about megamenu style, i.e. open it as a hierarchical list. But no need to grab all the other elements like description, freshness, image, etc?

• ​http://www.designchemical.com/media/images/custom_wordpress_mega_menu.jpg
• ​https://gaslampmedia.com/media/Screenshot-2015-03-14-at-5.04.28-PM.png
• ​http://www.blindfiveyearold.com/wp-content/uploads/2011/10/home-depot-mega-menu.png

[dcavins]

Replying to sooskriszta:

Replying to dcavins:

I'm still trying to figure out how I'd like to reflect hierarchy on the main groups directory--like BPGH with subgroup expands, simply including breadcrumbs with each result, or something else? The AJAX expand that BPGH uses seems to cause issues for users who expect to scan the list and find the group they're looking for, and never find second or deeper level groups because they don't show up. Maybe show them more like threaded comments with the option of closing groups of groups (could be a pretty expensive page load, and pagination would be odd)?

How about megamenu style, i.e. open it as a hierarchical list. But no need to grab all the other elements like description, freshness, image, etc?

• ​http://www.designchemical.com/media/images/custom_wordpress_mega_menu.jpg
• ​https://gaslampmedia.com/media/Screenshot-2015-03-14-at-5.04.28-PM.png
• ​http://www.blindfiveyearold.com/wp-content/uploads/2011/10/home-depot-mega-menu.png

I'm personally a fan of simplification, but I think that presenting the groups as a nested text list would be a pretty extreme departure from what we're currently doing... Happily, if we can make @hnla's snippets library idea work, it would be a simple matter for a variety of styles for that directory to be easy to achieve. Thanks for your feedback!

[hnla]

Years back I had a client project that used Group Hierarchy extensively, styling that was fairly complex, in fact the functionality was too. We can certainly try running up a series of style options that can be applied via the modules approach.

[dcavins]

I'm uploading a new patch that incorporates much of the helpful feedback provided here.

• Make sure the "parent" select is showing all possible groups by disabling pagination in the groups_get_groups() call.
• Fixed accessibility problems with form elements.
• Only show the "subgroups" tab if subgroups exist for that group. (This is a change from BPGH's behavior, but since I'm not proposing an override to the global bp_restrict_group_creation() for subgroups, it probably makes sense to always access the group creation screens from the same place.)
• Changed language to "Subgroups". End users can change it to whatever with a language file, of course. ;)
• Added hierarchy breadcrumbs to groups in the groups directory as the smallest possible change to the directory that shows hierarchical relationships. (Will attach image)
• Take the setting bp_restrict_group_creation() into account when calculating who can create subgroups.
• Add the parent setting to the settings screen for existing and in-creation groups.
• Don't show impossible options on the "who can create subgroups" setting if group creation is globally restricted.

I was thinking of building a walker to display the groups hierarchically like threaded comments, but there are some technical hurdles that lead me to think that you couldn't use bp_has_groups() to fetch the groups in the common way, and I'm concerned that people who've gotten used to working with that loop would be in for a surprise. I think we'd have to fetch all the groups and let the walker work it out. We'd also have to base pagination on the number of top-level groups for the current directory (groups with a parent_id of 0 for the main directory and with the parent_id of the current group for a subgroup directory). These things are doable but a major departure. If we wish to go this route, I think we should add a new template group-tree.php or similar because it will work very differently. (I'm happy to work on this, but I don't want to go down a path that we think is a bad idea.)

[dcavins]

Groups directory with hierarchy breadcrumbs.

[dcavins]

Second round.

[dcavins]

This ticket was the subject of useful and interesting conversation in the weekly meeting on August 3: ​https://wordpress.slack.com/archives/buddypress/p1470251765001059

[dcavins]

I'm attaching a minimal patch that adds no outward-facing functionality but adds the parent_id column to the groups database and support for querying by parent_id via groups_get_group(). These changes would make it much more straightforward to add hierarchical group functionality via a non-core plugin.

[dcavins]

Minimal patch that makes schema changes and support for querying by parent_id via groups_get_groups()

[DJPaul]

3961.03.patch looks OK to me. I would suggest adding the groups prefix onto the new bp_update_orphaned_groups_on_group_delete function, but that's about it.

Let's have some other review, then all we need is a few unit tests to: prove the bp_has_groups change, and that the orphaned-parent-group tidy-up works.

I think @r-a-y mentioned in Slack that we needed to maybe clear some Group caches. Perhaps he'll chime in here how best we can do that.

[DJPaul]

@dcavins I am probably misreading this but:

$child_group = groups_get_group( array( 'group_id' => $cgroup->id ) );

That block. Isn't $cgroup already the $child_group ??

[dcavins]

Replying to DJPaul:

@dcavins I am probably misreading this but:

$child_group = groups_get_group( array( 'group_id' => $cgroup->id ) );

That block. Isn't $cgroup already the $child_group ??

I'm sure you're right. That was one place where I was using my new convenience functions that I removed between .02 and .03 patches. So I made the change quickly and made sure phpunit passed, but haven't read the minimal patch thoroughly for new weirdness. Thanks for your careful review!

[dcavins]

Replying to DJPaul:

@dcavins I am probably misreading this but:

$child_group = groups_get_group( array( 'group_id' => $cgroup->id ) );

That block. Isn't $cgroup already the $child_group ??

Ah. It's the group data, but not the BP_Groups_Group object that we need to call save() on.

Re your earlier suggestions:
I'm attaching a slightly different version of the minimal patch that reinstates the tests I had written to check the behavior of parent_id queries in the BP_Groups_Group class and the orphan-tidying function. (Somehow I left them out of the minimal patch, too.) Please let me know if there are other tests that you can think of--I'm happy to write them. I've also prefixed the function name to be consistent as suggested: bp_groups_update_orphaned_groups_on_group_delete().

Thanks again for taking a look at the patch.

[dcavins]

Updated minimal patch that makes schema changes and support for querying by parent_id via groups_get_groups()

[r-a-y]

3961.05.patch includes cache invalidation so older cached objects are invalidated and refetched at run-time in the BP_Groups_Group::populate() method.

Includes one unit test - test_invalidate_old_group_cache_after_db_schema_change().

[DJPaul]

Ah. It's the group data, but not the BP_Groups_Group object that we need to call save() on.

Maybe that's something that could be looked at for being changed in the future

[DJPaul]

The patch is looking really close. Question for @r-a-y:

The populate change you made to invalidate old caches. Doing this here strikes me a little odd, because this is basically a one-time (per group) only thing. Once all the groups have been cleared, it'll never get used again.

Did you consider fetching all Group IDs during the database upgrade routine, and clearing those there?
We don't need to worry about re-building the cache there, and I'd even suggest a raw SQL query to fetch the IDs if groups_get_groups doesn't support such an optimal query (I haven't looked at its query builder for a while).

[DJPaul]

sorry for messing up the CCs

[r-a-y]

The populate change you made to invalidate old caches. Doing this here strikes me a little odd, because this is basically a one-time (per group) only thing.

Writing up some upgrade code would probably take up more lines and that's also a one-time use case.

My small addition means that the cache is only purged when needed. Also, looping through all the groups can be intensive depending on the number of groups. Our upgrader isn't mature enough to handle taxing situations yet (#6841).

[DJPaul]

The upgrader code is all about being used once, right? update_to_x_version is only called if you're on an older version.

The performance concern is why I said a direct SQL query to only return the IDs avoids any of the (unintentional) overhead of using the existing groups_get_group function*. It also avoids out-of-date cached information being returned, when the whole point is to clear the cache. It's about the only place I'd ever use a direct SQL query.

Unless someone is going to commit to "fixing" the upgrader code in 2.7 or 2.8 -- of which I've seen no significant interest in recently -- worrying about giant-scale BP sites, is beginning to feel like an edge case excuse.

(*The function may already have great SQL for returning IDs only, and for forcing a query instead of using a cache. I haven't looked.)

[dcavins]

I think @r-a-y's invalidation scheme makes sense and wouldn't be costly. If we must do it at upgrade, the simplest code is probably:

<?php
function bp_update_to_2_7() { 
        bp_add_option( 'bp-emails-unsubscribe-salt', base64_encode( wp_generate_password( 64, true, true ) ) ); 
        /* 
         * Also handled by `bp_core_install()`. 
         * Add `parent_id` column to groups table. 
         */ 
        if ( bp_is_active( 'groups' ) ) {
                bp_core_install_groups();
                
                // Invalidate all cached group objects.
                global $wpdb;
                $bp = buddypress();

                $group_ids = $wpdb->get_column( "SELECT id FROM {$bp->groups->table_name}" );
                foreach ( $group_ids as $group_id ) {
                        wp_cache_delete( $group_id, 'bp_groups' );
                }
        } 
} 

which seems pretty straightforward, but I don't know how efficient wp_cache_delete() is. @r-a-y or @boonebgorges probably know something about it. :)

[DJPaul]

wp_cache_* are not sets of functions you ever need to worry about performance with.

[dcavins]

Add at-upgrade groups cache busting routine.

[dcavins]

I've just added a new patch that adds an at-upgrade cache busting routine. I tested it against a local site (hosted on my laptop with a fresh memcache setup) with 1500 groups, and fired the upgrade by visiting the main WP Dashboard. Processing the 1500 cache entries (and running the ALTER commands against the db) added about 2 seconds to the page load and many queries. So I think it's OK, assuming that BP sites with that many groups are running on more robust servers than my laptop, ha ha.

I think this is ready to go in. Comments welcome.

[boonebgorges]

A couple notes:

• The if ( ! empty( $r['parent_id'] ) ) { check in BP_Groups_Group::get() will make it impossible to fetch groups with parent=0. This should be a stricter check of some sort. I suggest making the default value null or false, and then changing the check here to if ( null !== $r['parent_id'] ). Tests demonstrating this behavior would also be excellent.

• It would be nice to have @since documentation where new params are added to existing functions.

• There seems to be an indentation issue in bp_update_to_2_7()

[sooskriszta]

Needless to say, I am super pumped to see this being merged into core.

One backpat-type request though: Could the core possibly:

• Check whether ​https://wordpress.org/plugins/bp-group-hierarchy/ is installed.
• If yes, then migrate groups over to the core functionality and prompt admin to disable that plugin.

[dcavins]

Replying to sooskriszta:

Needless to say, I am super pumped to see this being merged into core.

One backpat-type request though: Could the core possibly:

• Check whether ​https://wordpress.org/plugins/bp-group-hierarchy/ is installed.
• If yes, then migrate groups over to the core functionality and prompt admin to disable that plugin.

Thanks for your comment. This change will not replace the functionality of BP Group Hierarchy. This change adds core support that will make it much easier to write an updated plugin like BPGH. I have most of the code written so will assemble it into a standalone plugin. I'd appreciate your help in testing it once I've had a chance to put it together. Needless to say, it will require at least BP 2.7. :)

[dcavins]

Hi @boonebgorges, thanks for your comments.

• Yes, the empty() check was _totally_ wrong. Using nullworks nicely. I've added a test for querying for top-level groups.
• I've added the @since documentation to groups_get_groups() and BP_Groups_Group::get()

I'm adding a new version of the patch that addresses these issues. Thanks again!

[dcavins]

Address Boone's comments. Also, groups_get_groups() returns BP_Groups_Group objects now.

[boonebgorges]

Thanks, @dcavins! A couple more things (one of which is caused by my previous suggestion ;) )

• In BP_Groups_Group::get(), you're checking to see whether parent_id is null. But the default value you're passing through to the rest of the chain - bp_has_groups() etc - is false. This is going to get converted to a 0 for the query, which is not what we want. The null value should either be null or false, but it needs to be consistent.

• In bp_groups_update_orphaned_groups_on_group_delete() (and maybe elsewhere?) you do this:

$grandparent_group_id = ! empty( $group->parent_id ) ? $group->parent_id : 0;

We should enforce that group objects have a numeric parent_id (populate()). This way, the only way parent_id could be empty is if it's 0, in which case you can skip the check and say $grandparent_group_id = $group->parent_id.

[dcavins]

Hi @boonebgorges- Thanks again for taking the time to look over this patch. I agree that I was sloppy in the last patch and the value of parent_id needs to be consistent everywhere. I'm leaning toward using null to mean "do nothing" everywhere; passing false will be interpreted to mean "get the top level groups" just like passing 0 for the parent_id (wp_parse_id_list() converts false and null to 0). It could just as well be false everywhere if that makes more sense to you (then null would be interpreted as 0).

I've updated the patch to be more consistent and added more tests for bp_has_groups() and for BP_Groups_Group::get().

[dcavins]

Make sure that the default parent_id everywhere is null. Add tests for null and false values

[boonebgorges]

I prefer null for this purpose too, for just the reasons you've outlined.

The updated logic looks good to me. If your testing is clear, let 'er rip.

[dcavins]

In 11095:

Add query support for hierarchical groups.

Adds a new column to the groups table, parent_id, and support for
querying by parent_id via groups_get_groups() and
bp_has_groups().

Does not add any front-facing hierarchy functionality—that is left for
now to plugins.

Props dcavins, boonebgorges, r-a-y, djpaul, mercime.

See #3961.