Skip to:

Opened 12 years ago

Closed 12 years ago

#4134 closed defect (bug) (fixed)

bug in activity filtering for join group

Reported by: gwu123's profile gwu123 Owned by:
Milestone: 1.6 Priority: normal
Severity: normal Version: 1.5.5
Component: Activity Keywords: needs-patch
Cc: gwu123

Description (last modified by DJPaul)

The activity filter has a bug when filtering “join group” activity.

Steps for recreating this scenario

  1. Join a group from Group directory (not from the group home page)
  2. Check the “My Groups” activity. You will see an entry for “user joined group….”
  3. Now check the “All members” activity tab. This will not capture the above activity.

Change History (7)

#1 @DJPaul
12 years ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 1.6
  • Version set to 1.5.5

Confirmed. Probably a hide_sitewide issue.

#2 @gwu123
12 years ago

  • Cc gwu123 added

#3 @chriskeeble
12 years ago

(Apologies if including code like this is not the way to make suggestions, I'm not too familiar with the process here).

When the join group button is clicked...
1) on an individual group page: the 'hide_sitewide' property for the recorded activity is set to false
2) on the Groups (list) page: the 'hide_sitewide' property for the recorded activity is set to true

In case 2 above, $bp->groups->current_group is not set (there is no current group). The function groups_join_group() handles this:

if ( !isset( $bp->groups->current_group ) || !$bp->groups->current_group || $group_id != $bp->groups->current_group->id )
    $group = new BP_Groups_Group( $group_id );
    $group = $bp->groups->current_group;

But then the function groups_record_activity() uses $bp->groups->current_group to determine whether to set hide_sitewide to true or false...

// If the group is not public, hide the activity sitewide.
if ( isset( $bp->groups->current_group->status ) && 'public' == $bp->groups->current_group->status )
    $hide_sitewide = false;
    $hide_sitewide = true;

I think the test should be using the item_id (group id) from $args, load the specified group and test its status, such as this (which works)...

function groups_record_activity( $args = '' ) {
	global $bp;

	if ( !bp_is_active( 'activity' ) )
		return false;

	// If the group being joined is specified, use it
	if ( isset( $args ) && array_key_exists( 'item_id', $args ))
		$group = new BP_Groups_Group($args['item_id']);
		$group = $bp->groups->current_group;	// Otherwise it's the current group - but item_id won't be set in the activity ??

	// If the group is not public, hide the activity sitewide.
	if ( isset( $group->status ) && 'public' == $group->status )
		$hide_sitewide = false;
		$hide_sitewide = true;

	$defaults = array (
		'id'                => false,
		'user_id'           => $bp->loggedin_user->id,
		'action'            => '',
		'content'           => '',
		'primary_link'      => '',
		'component'         => $bp->groups->id,
		'type'              => false,
		'item_id'           => false,
		'secondary_item_id' => false,
		'recorded_time'     => bp_core_current_time(),
		'hide_sitewide'     => $hide_sitewide

	$r = wp_parse_args( $args, $defaults );
	extract( $r );

	return bp_activity_add( array( 'id' => $id, 'user_id' => $user_id, 'action' => $action, 'content' => $content, 'primary_link' => $primary_link, 'component' => $component, 'type' => $type, 'item_id' => $item_id, 'secondary_item_id' => $secondary_item_id, 'recorded_time' => $recorded_time, 'hide_sitewide' => $hide_sitewide ) );

BUT - I'm concerned that if item_id is ever not set explicitly in $args, then an activity will be recorded with a false (zero) item_id (although this is already the case).

Any thoughts?

#4 @chriskeeble
12 years ago

Update: I've done a check through all calls to groups_record_activity() and item_id is being set in all cases, as it should be. So may be better to always use it to grab the group for clarity & simplicity in the code.

#5 @DJPaul
12 years ago

  • Keywords needs-patch added

I'd suggest that we check for $bp->groups->current_group->id == $'item_id (taking care to handle if item_id is not set, of course). If it does, we can use that group object again, which will save a database query. If you are able to make a patch, that would be helpful.

#6 @boonebgorges
12 years ago

+1 to what DJPaul said.

  • Before querying for the group object, check to see whether $item_id == bp_get_current_group_id(), and if so, then use groups_get_current_group() to populate the $group object.
  • Don't instantiate BP_Groups_Group directly. Use groups_get_group( array( 'group_id' => $group_id ) ) instead. (This is a new change to BP 1.6, to allow for better caching.)

#7 @boonebgorges
12 years ago

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

(In [6097]) Improvements to the way the hide_sitewide default is determined in groups_record_activity()

groups_record_activity() should attempt to use the item_id passed to the
function when determining the default value of 'hide_sitewide', instead of the
current group id (which will not always be populated, eg in the groups

Fixes #4134

Props chriskeeble

Note: See TracTickets for help on using tickets.