Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 10 years ago

#5411 closed defect (bug) (fixed)

Members nav of single group : total members seems not set

Reported by: imath's profile imath Owned by: boonebgorges's profile boonebgorges
Milestone: 2.0 Priority: normal
Severity: normal Version:
Component: Groups Keywords: has-patch commit
Cc:

Description

In a single group the Members navigation includes a span to show the amount of members belonging to the group.

I think since changeset 7898 that this members count is no more set. As a result, the Members navigation keeps showing 0.

Attachments (4)

5411.diff (1.0 KB) - added by imath 10 years ago.
5411.02.patch (2.8 KB) - added by r-a-y 10 years ago.
5411.02b.patch (3.5 KB) - added by r-a-y 10 years ago.
5411.list_admins_mods.01.patch (1.9 KB) - added by r-a-y 10 years ago.

Download all attachments as: .zip

Change History (25)

@imath
10 years ago

#1 @boonebgorges
10 years ago

Good catch, thanks. Moving the groups_get_groupmeta() query is a good idea for a different reason (see #5398 and the change I'm about to commit). I'll do something a bit different to account for the current issue.

#2 @boonebgorges
10 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 7934:

Set default args for BP_Groups_Group::construct()

Must ensure that the 'populate_extras' param defaults to true when the class
is instantiated directly.

Fixes member counts in navigation.

Fixes #5411

#3 @boonebgorges
10 years ago

Actually, I changed my mind about moving the groupmeta calls :) My original thought was that it only takes a single SQL query to fetch all groupmeta, so there wasn't much harm in doing both of the groupmeta calls even when populate_extras was not true. However, I'm now thinking that there are still cases where it's not necessary to have this info at all, so why waste the DB call?

@r-a-y
10 years ago

#4 @r-a-y
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

r7934 defaults the 'populate_extras' parameter to true.

However, there are very few instances where we need the extra group data. We only need it to be true when on a single group or in a group loop.

This can save a lot of unnecessary queries since groups_get_group() or new BP_Groups_Group is called quite a bit.

02.patch reverts r7934 and fixes a few places where we should be using the current_group object in BP_Groups_Template.

#5 @boonebgorges
10 years ago

I agree that there are many places where we do not need the extra group data. However, I think that defaulting to populate_extras=false will break backward compatibility with plugins that may be depending on the populate_extras data to be there. So I think we'll have to do the opposite of what you've done here: pass populate_extras=false in the cases where it's *not* needed.

#6 @r-a-y
10 years ago

However, I think that defaulting to populate_extras=false will break backward compatibility with plugins that may be depending on the populate_extras data to be there.

I just checked all wp.org plugins and only one plugin suffers from this change no plugin suffers from this change :).

bp-groups-suggestions expects the group extras so the last active timestamp will work:
https://github.com/wp-plugins/bp-groups-suggestions/blob/9271125afe24e5b18f9f0d7fcbae9d6d563e679a/bp-group-suggest.php#L234

Update: bp_get_group_last_active() is smart enough to grab the last active timestamp from groupmeta when it doesn't exist in the global.

FYI, if you're interested, here's what I used to search all wp.org plugins on Github:
https://github.com/search?q=user%3Awp-plugins+groups_get_group%28&type=Code
https://github.com/search?q=user%3Awp-plugins+BP_Groups_Group%28&type=Code

Last edited 10 years ago by r-a-y (previous) (diff)

#7 @boonebgorges
10 years ago

Whoa, sweet. Never thought to use the github mirror for searching! Thanks for the tip!

You've convinced me :) I'm happy to go ahead with the proposed change, as long as the tests pass.

#8 @r-a-y
10 years ago

I'm happy to go ahead with the proposed change, as long as the tests pass.

So there's one test that fails because it is expecting 'populate_extras' to be true. I've attached a patch that alters the test to add the 'populate_extras' parameter.

I hope this isn't a big enough dealbreaker so we'll have to do the opposite approach of the patch!

@r-a-y
10 years ago

#9 @boonebgorges
10 years ago

  • Keywords commit added

.02b looks good to me, r-a-y. It's testing something that only needs to be tested when 'populate_extras' is true. Thanks!

#10 @r-a-y
10 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 8056:

Groups: When fetching a new group object, default 'populate_extras' to false.

In r7934, the 'populate_extras' parameter was set to true. This
parameter queries additional info about the group including group
admins, moderators and member status.

There are very few instances where we need this extra information. We
only need this data when on a single group or in a group loop.

Therefore, this commit sets the 'populate_extras' parameter to false
by default and sets it to true when setting up the current group
object.

Fixes #5411

#11 @dcavins
10 years ago

Could this change be breaking $groups_template object creation in BP Group Hierarchy? Here's an example of the $groups_template object, captured within the function bp_group_list_admins() (my BP is up to r8063).

BP_Groups_Hierarchy_Template Object
(
    [vars] => Array
        (
            [params] => Array
                (
                )

        )

    [current_group] => 0
    [group_count] => 1
    [groups] => Array
        (
            [0] => stdClass Object
                (
                    [group_id] => 17
                )

        )

    [group] => BP_Groups_Hierarchy Object
        (
            [vars] => Array
                (
                    [parent_id] => 15
                    [true_slug] => captains-and-cormorants
                    [path] => category-test/captains-and-cormorants
                )

            [id] => 17
            [creator_id] => 1
            [name] => Captains and cormorants
            [slug] => category-test/captains-and-cormorants
            [description] => Especially for you other types
            [status] => private
            [enable_forum] => 1
            [date_created] => 2013-08-08 14:58:56
            [admins] => 
            [mods] => 
            [total_member_count] => 
            [is_member] => 
            [is_invited] => 
            [is_pending] => 
            [last_activity] => 
            [user_has_access] => 
            [args:protected] => 
        )

    [in_the_loop] => 1
    [pag_page] => 1
    [pag_num] => 20
    [pag_links] => 
    [total_group_count] => 1
    [single_group] => 1
    [sort_by] => 
    [order] => 
)

Could be totally unrelated, but the problem appeared at about the same time.

#12 follow-up: @r-a-y
10 years ago

The problem can be traced back to the introduction of the 'populate_extras' parameter in r7898.

The BP_Groups_Hierarchy class does not have this parameter when it constructs its group since this is brand-new.

What we could do is introduce backwards compatibility support to fill in the group admins and mods when they are empty in the bp_group_list_admins() function, so when it is called, the group admin / mods will be shown.

Update: There are some other problems besides just fetching the group admin / moderators. I've just informed ddean of the issues here:
https://github.com/ddean4040/BP-Group-Hierarchy/issues/6

Last edited 10 years ago by r-a-y (previous) (diff)

#13 in reply to: ↑ 12 @dcavins
10 years ago

Replying to r-a-y:

The problem can be traced back to the introduction of the 'populate_extras' parameter in r7898.

The BP_Groups_Hierarchy class does not have this parameter when it constructs its group since this is brand-new.

What we could do is introduce backwards compatibility support to fill in the group admins and mods when they are empty in the bp_group_list_admins() function, so when it is called, the group admin / mods will be shown.

Update: There are some other probems besides just fetching the group admin / moderators. I've just informed ddean of the issues here:
https://github.com/ddean4040/BP-Group-Hierarchy/issues/6

r-a-y: You're awesome. Thanks for looking into this.

#14 @boonebgorges
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This makes me feel not so bad about being the backward compatibility ogre :)

r-a-y, while obviously it's good for ddean to update bp-groups-hierarchy (my guess for the most popular plugin that'll be affected by these changes), we should try to accommodate them as much as possible. I'd guess that your suggestion for bp_group_list_admins() is going to be good enough for most purposes; hopefully not many people are attempting to access that part of the global directly.

Off topic, but what are the aspects of bp-group-hierarchy that are broken by the groupmeta changes? Is it just performance/caching issues? If there are breaking changes, could I ask you to open a separate ticket with details? The migration to WP metadata is supposed to be seamless :)

#15 @r-a-y
10 years ago

I'd guess that your suggestion for bp_group_list_admins() is going to be good enough for most purposes; hopefully not many people are attempting to access that part of the global directly.

We can definitely do that.

However, the issue with BP Group Hierarchy is it already uses two arguments in its constructor class. However, BP_Groups_Group just added a second parameter (r7898), so that causes some issues that could only be solved if ddean added a third parameter to his constructor or just ignores the populate_extras parameter and pastes all the extra group data calls into his BP_Groups_Hierarchy::populate() method. (I don't like the latter option I recommended!)

There might be other ways to solve the issue, but that's what I gathered from my quick audit of BP Group Hierarchy.

Off topic, but what are the aspects of bp-group-hierarchy that are broken by the groupmeta changes?

BP Group Hierarchy copies most of the BP_Groups_Component class. Because of that, ddean will need to copy in the changes to add the meta_tables parameter from r7840 since it won't be there in the current version of his plugin.

It's a simple fix.

Last edited 10 years ago by r-a-y (previous) (diff)

#16 @boonebgorges
10 years ago

Ah, thanks, r-a-y. IIRC, we added the ability to override BP_Groups_Group just for ddean, so it's pretty unlikely that anyone else is using it. If you're overriding core classes, then caveat emptor I guess.

ddean will need to copy in the changes to add the meta_tables parameter

Gotcha. I guess that falls into the same category.

#17 @r-a-y
10 years ago

Well, I ended up sending a pull request to ddean since I couldn't help myself!

If you're interested:
https://github.com/ddean4040/BP-Group-Hierarchy/pull/7

#18 @boonebgorges
10 years ago

Thanks, r-a-y :)

Would you mind writing a short bpdevel post that explains this situation, and maybe links to this ticket and/or your pull request? I feel like then we'll have done our due diligence for people who are doing wacky things with the groups component. (I also think we should do the admins_mods fix, as that may affect more people than just ddean.)

#19 @r-a-y
10 years ago

list_admins_mods.01.patch adds backpat support when using the bp_group_list_admins() or bp_group_list_mods() functions if 'populate_extras' is false or does not exist.

One thing with the patch is I've set the $args visibility in BP_Groups_Group to public so I can access it. It's either that or create a public method to access the protected property.

I'll write up a short bpdevel post a bit later about the groups component changes.

#20 @boonebgorges
10 years ago

Hi r-a-y - That patch seems fine. In my mind, I guess I'd been thinking something more along the lines of:

if ( empty( $group->admins ) ) {
    $group = groups_get_group( array(
        'group_id' => $group->id,
        'populate_extras' => true,
    ) );
}

This prevents us from having to examine arguments for original intent, and also keeps all the query logic centralized in the BP_Groups_Group class. But it's a fairly small difference from what you're doing (and anyway, it should rarely be triggered) so whatever you think is best is OK with me.

Thanks for doing this!

Last edited 10 years ago by boonebgorges (previous) (diff)

#21 @r-a-y
10 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 8168:

Groups: Add backward compatibility when fetching list of admins/mods.

In r8056, the change was made so a group does not fetch its extra data
like grabbing the list of group admins and mods by default.

However, if a group template loop was created with the 'populate_extras'
parameter set to false and either the bp_group_list_admins() and
bp_group_list_mods() functions were used in the loop, these functions
would not output the correct list.

To solve this, this commit manually fetches the group admins / mods if
the 'populate_extras' flag in the group template loop is set to false.

Fixes #5411

Note: See TracTickets for help on using tickets.