Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 7 years ago

Last modified 7 years ago

#6356 closed defect (bug) (fixed)

Filtering the group invites list

Reported by: henrywright's profile henry.wright Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.2.1
Component: Groups Keywords: needs-testing has-patch 2nd-opinion
Cc:

Description

When filtering the list of potential friends that can be invited to join a group, if the filter causes the number that can be invited to be 0, then the invite list on the invites page looks blank (see attached screenshot).

bp_friends_get_invite_list is the hook I'm using.

In such cases, a message such as "There's nobody available to invite at this time" would be useful for the end-user to see.

Attachments (3)

invite-step.jpg (17.8 KB) - added by henry.wright 10 years ago.
6356.01 (1.2 KB) - added by hnla 10 years ago.
Add affitional check for get_new_invite_list to prevent invite loop being called if friends have been filtered - suggested patch only.
6356.02.patch (1.2 KB) - added by hnla 9 years ago.
re-fresh patch

Download all attachments as: .zip

Change History (30)

#1 @hnla
10 years ago

Hmm I re-factored these templates a while back and thought I had accounted for all states, can you check:
#5509
Maybe something obvious was overlooked.

#2 @henry.wright
10 years ago

Hey @hnla

Doing the following in the invites-loop.php template file assumes bp_get_new_group_invite_friend_list() will always return <li> items:

<ul>
    <?php bp_new_group_invite_friend_list(); ?>
</ul>

In my case, it returns false, so the output on the page is:

<ul>

</ul>

#3 @hnla
10 years ago

and in my original patch 5509-invites-loop.patch I did wrap that in a check, but the logic checking was subsequently moved to the parent template send-invites.php so something went awry in that template as that's really where we need/needed to logically run the checks. I'll need to re-visit send-invites.php and work out where it went wrong as was sure originally all conditions correctly checked for friends or all friends already members of the group as returning null results.

#4 @henry.wright
10 years ago

I see what you did in that patch. Doing that would resolve the problem.

Perhaps the plan was to keep templates streamlined, with such logic happening somewhere upstream?

I think the problem is with bp_get_new_group_invite_friend_list() returning false. Perhaps instead of returning false, a possible solution would be for it to return a string.

#5 @hnla
10 years ago

Perhaps the plan was to keep templates streamlined, with such logic happening somewhere upstream?

But yes that's the point, it's what I'm referring to in above comments re the send-invites.php template, if you check that template you'll see it's in effect the master template calling the invite loop, as I had things in my second pass I ran all the checks there so never called the loop unless there was a reason to i.e it returned true - in a subsequent pass things were changed slightly and think the conditional checks aren't robust enough, so probably need to re-visit and re-implement my original conditionals.

I'll take a look and think how best to handle, perhaps simply adding in a further check in invites loop but don't really like duplicating stuff - good spot though, it passed me by :)

#6 @DJPaul
10 years ago

  • Milestone changed from Awaiting Review to 2.3

#7 @hnla
10 years ago

@henry.wright
Henry can you describe the steps you took to achieve this result.
In my re-testing of the original patch the results are still as intended:

  • If I have no friends, appropriate message displayed to that affect
  • If I have friends yet they are all already members of group, appropriate message displayed

In both circumstances we have passed over the get invite loop to the second and third conditions so not sure how you were able to see the invite loop in the left column ul parent elements.

When you mention filtering on bp_friends_get_invite_list what exactly are you running so I can test same conditions.

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

#8 @henry.wright
10 years ago

@hnla here's an example filter that would result in the screenshot I posted:

/**
 * Filter the list of potential friends that can be invited to the group.
 */
function bptrac_filter_group_invite_list( $friends, $user_id, $group_id ) {

        // Filter the friends array so that there's 0 friends available to invite.
        $friends = array();

	return $friends;
}
add_filter( 'bp_friends_get_invite_list', 'bptrac_filter_group_invite_list', 10, 3 );

#9 @hnla
10 years ago

But steps?
Why are you running this and how and what screen are you viewing, it feels to me you are testing outside of actual normal process flow?

If I have an existing list of sent invites in view in a group and I run that filer then the send-invites.php conditional checks kick in and I have the 'no friends' message displayed.

#10 @henry.wright
10 years ago

The need to filter the invites list came about during development of a plugin imath and I were working on for CF Community. We are restricting group membership according to member type. See here:

Ref https://github.com/CFCommunity-net/buddypress-group-restrictions/blob/master/includes/functions.php#L247

Imagine the scenario where a member who created the group has 100 friends, but none of which are eligible to join the group because their member type isn't allowed by that group. Hence my filter returns 0 eligible friends that are available to invite.

#11 @hnla
10 years ago

:) Ok sorry to go on about this but... so we know the why, but I am still not understanding how you bypass the send-invites.php checks, it's that which is confusing me, sure it's me just missing something but without that understanding I can't see where a further check would need to go (it could be wrapped directly around the invite-loop but shouldn't have to be really)

#12 @henry.wright
10 years ago

Sorry for not explaining properly :)

With the bptrac_filter_group_invite_list() function I posted above in functions.php, try the following steps:

  1. Go to groups/create/step/group-details/ and begin to create a new group.
  2. Complete steps 1 - 3.
  3. Now we're at step 4. You should see the screenshot I posted above. The message on the page should read: "Select people to invite from your friends list."

The file the message is coming from isn't send-invites.php. It's coming from create.php See here: https://github.com/buddypress/BuddyPress/blob/master/src/bp-templates/bp-legacy/buddypress/groups/create.php#L186

The relevant condition in that file doesn't take filtering into account because it just checks the total friend count is > 0:

if ( bp_is_active( 'friends' ) && bp_get_total_friend_count( bp_loggedin_user_id() ) ) :

Hence why you see the text "Select people to invite from your friends list"

#13 @hnla
10 years ago

Right - so that just needs bp_new_group_invite_friend_list() adding to the friend count check but there ought to be a more elegant way of performing the check. ( I should have paid more attention to the subnav in SC)

Version 0, edited 10 years ago by hnla (next)

#14 @henry.wright
10 years ago

You could just change the condition from:

if ( bp_is_active( 'friends' ) && bp_get_total_friend_count( bp_loggedin_user_id() ) ) :

to

if ( bp_is_active( 'friends' ) && friends_get_friends_invite_list() ) :

That means if friends_get_friends_invite_list() returns false then the following text will be shown:

Once you have built up friend connections you will be able to invite others to your group.

But then, it might be worth changing that text to something slightly more generic because it doesn't quite make sense for all scenarios.

Last edited 10 years ago by henry.wright (previous) (diff)

@hnla
10 years ago

Add affitional check for get_new_invite_list to prevent invite loop being called if friends have been filtered - suggested patch only.

#15 @DJPaul
10 years ago

  • Keywords has-patch needs-testing added
  • Milestone changed from 2.3 to 2.4

#16 @DJPaul
9 years ago

  • Keywords needs-refresh added; has-patch removed

@hnla
9 years ago

re-fresh patch

#17 @hnla
9 years ago

  • Keywords has-patch added; needs-refresh removed

Updated patch.
This needs testing and needs attention to verbiage added for new elseif message condition.

#18 @DJPaul
9 years ago

  • Milestone changed from 2.4 to Future Release

@henry.wright Can you re-check this issue with BuddyPress trunk? I can't recreate the problem, maybe it was fixed already somehow.

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


9 years ago

#20 @henry.wright
9 years ago

@DJPaul

Just tested this. It doesn't seem to have been fixed. If you use something like the filter I posted here, you should be able to see the problem exists in trunk.

#21 @DJPaul
9 years ago

  • Keywords 2nd-opinion added

Still can't repro. Am using your filter.

On trunk, my test user has a friend, I add your filter, I go to Send Invites in a Private Group, I see "All of your friends already belong to this group." and no side bar.

Last edited 9 years ago by DJPaul (previous) (diff)

#22 @henry.wright
9 years ago

You'll need to have some friends who don't belong to the group. So perhaps friend 3 or 4 people, make sure none of those already belong to the group and then try again?

#23 @DJPaul
7 years ago

  • Milestone changed from Future Release to 3.0

Let's test this abasing Nouveau and see what happens, and close or fix appropriately.

#24 @DJPaul
7 years ago

Lol - not "abasing" - I meant "against" ;)

#25 @hnla
7 years ago

Fresh from abasing Nouveau ( had thought you meant abusing Nouveau which seemed really unfair) I can report that as Nouveau invites are a veritable re-write from head to toe and backboned we don't suffer this issue, it was really specifically a problem we had with Legacy's template.

I think this ticket is now old enough that we should close but perhaps we ought to run one test over legacy to verify what UX we have got as there were a series of changes.

#26 follow-up: @DJPaul
7 years ago

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

That's good news. Closing this because it works in Nouveau and if anyone wants to patch Legacy, be our guest.

#27 in reply to: ↑ 26 @henry.wright
7 years ago

Replying to DJPaul:

...Closing this because it works in Nouveau...

Yay!

Note: See TracTickets for help on using tickets.