Friendship activity missing

[dontdream]

I just found what I think is a bug. These are the steps to reproduce, on a new installation of WordPress 4.0.1, BuddyPress 2.1.1, all components enabled.

1. User first requests the friendship of user second.

2. User second accepts the request.

3. On the page members/first, user second can see the activity first and second are now friends.

4. On the page members/second, user second can see the activity second and first are now friends.

5. On the page members/first, user first and other users can see the activity first and second are now friends.

6. On the page members/second, user first and other users can't see any friendship related activity.

The last point was unexpected to me. I know that one of the two generated friendship activities is hidden sitewide, but should it be hidden on the profile of second?

[dontdream]

I'm trying to understand this issue better, and this is what I found in the code:

1. A friendship generates two activities, one for each friend.

2. One of the twin activities is hidden, to avoid (I think) showing two activities for the same friendship in the main activity page.

Now the problem is, there is no simple way to decide whether to show the hidden friendship activity or not. It should remain hidden when its twin activity is shown, and it should be displayed when its twin is not shown. I wasn't able to find a simple way to code this condition.

Alternative solutions could be:

1. Don't hide the second activity, and live with the nuisance of seeing (sometimes) two activities for a friendship (e.g., first obtained the friendship of second, second accepted the friendship of first).

2. Generate a single activity for a friendship, and modify the activity loop (how?) to make it find the friendship activity for both the friends.

By the way, I'm surprised I am the first to notice this issue. Am I missing something?

[imath]

Thanks for your feedback @dontdream

I'm exploring your ticket since yesterday night. Working on it :)

I think this situation appeared after 1.1 release.

[imath]

Since ​BuddyPress 1.1, 2 activities are generated when a friendship is confirmed. 1 for each user of the relationship. While the activity for the initiator is "public", the one for the other user is hide_sitewide = true.

As the type for these 2 activities is the same, and the actions are very similar (u1 and u2 are now friends and vice versa). I personaly think it makes sense to only have 1 activity shown into the activity directory. So the hide_sitewide = true use is interesting from this point of view :

• it makes sure only one activity is displayed site wide
• it makes sure each user will see an activity informing they are now friends with each other.

dontdream reported that when a user check the user's profile of a != initiator user, then the activity is not displayed, which is due to the hide_sitewide field: so it's logic (not saying it's necessarly consistent).

I think we have 2 options :

1. leave this as is, but then we'll need to open a new ticket to fix a problem i've found when a ​friendship is removed (activities are never deleted because the action 'friendship_accepted' seems to be no more in use since 1.1)
2. review the way friendship is tracked into the activity streams (directory & users) and to fix the issue the way dontdream is expected it to be.

In ​6040.patch, I've worked on the second option. IMHO, i think from a "tracking" perspective it can be interesting to display 2 activities in the activity directory. The first one will inform someone requested another user to be his friend. Later when the "another user" accepts the friendship we can publish a second activity informing he accepted the friendship.

Till the friendship is confirmed the initiator will have an activity saying "u1 resquested u2 to be their friend" into his profile :

As soon as u2 accepts the friendship, then using the format callback of the activity action, we can change the action string to u1 & u2 are now friends

So to summarize, when on activity directory: the 2 activities are now different (requested/accepted) and when on a user's profile: it depends if the friendship is confirmed, but in the end both users will have a public activity (which is what is expected by this ticket).

Then, i also worked on some other cases about activities :
when a friendship is withdrawn: delete the 'friendship_created' activity
when a friendship is rejected: delete the 'friendship_created' activity
when a friendship is removed: delete the 2 activities 'friendship_created' & 'friendship_accepted'

I've created unit tests for each cases. I haven't set any milestone because :

If we are to choose option 2, we will need to create an upgrade routine that will get all activities having

array(
	'component'   => buddypress()->friends->id,
	'filter'      => array( 'action' => array( 'friendship_created' ) ),
	'show_hidden' => true
);

and update them to

array(
	'type'          => 'friendship_accepted',
	'hide_sitewide' => false
);

Without an upgrade routine all activities would have an action like 'u1 requested u2 to be their friend'

Option 1 is the choice that exists so far in BuddyPress, and it can make sense to stay with it (as soon as we fix the delete activity issue).

I personaly think option 2 is interesting for a community administrator as when he filters the activity stream to friendships, he has activities with the same action (u1 & u2 are now friends and vice-versa) in double. But we can think about it for a future release and only fix the 'delete' issue for 2.2 (in a new ticket).

What do you think ?

[dontdream]

@imath thank you for the extensive review, I like your approach to option 2.

[DJPaul]

For historical reasons, I am pretty hesitant with the idea of changing hide_sitewide on existing fields. hide_sitewide, from memory, does two different things, and the complexity of separating those apart into separate columns (or however they'd best be re-implemented) is one reason I think we haven't touched that part of the code in a few years. We'd need to be absolutely certain that we wouldn't be causing some other problem, regardless of having unit tests.

I think we'd also want to test such an idea against the old testbp.org database as that had run every version of BuddyPress ever, and had some fun things like (I think) old columns we dropped back in the very early days, etc.

[boonebgorges]

Like DJPaul, I'm hesitant. The original reasoning behind having two separate items is this:

• When a new friendship is created, we should show a new activity item on each person's activity stream
• User activity streams are basically of the form bp_has_activities( 'user_id' => bp_displayed_user_id() )
• But an activity item can only have one user_id, so we need two separate items
• But if we have two separate items, we'll have near-duplicates in the sitewide stream, so we make one of them hide_sitewide

The 'hide_sitewide' stuff is a huge can of worms. See #3857. I think that if we start changing this value on existing items, we're going to cause all sorts of problems for existing installations, especially those that are doing custom queries.

Perhaps a better way forward is this: #4988 will make it possible to do more complex activity stream queries. So perhaps we can have a query that looks like this when looking at a user's activity stream: show all activities where user_id = this user OR ( user_id != this_user && component = 'friends' && item_id = this_user ). Then, we stop creating the second 'hide_sitewide' item, and maybe even clear the old ones out of the database on upgrade (or at least stop returning them in normal queries).

[dontdream]

@boonebgorges I like the idea of generating a single activity for a friendship.

The only issue I see is with the activity ownership, e.g. in the 'delete' feature. Should any of the two friends be able to delete the activity? Should he need the consent of the other one?

Currently each activity has a single owner, while the new friendship activity will have two owners and will thus require a special treatment in a few places.

[imath]

Thanks a lot for your feedback DJPaul, boonebgorges and dontdream.

I also like the idea of boonebgorges. So i made some tests and had a quick overview about the work r-a-y and boonebgorges have done on #4988: that's pretty awesome :)

The main problem in our case, which is also a problem when we use WP Meta Queries is that, there's no way to have an 'OR' keyword between the main sql parts and the meta query sql parts. It's always a 'AND' keyword.

Let's stay in BuddyPress: in the get method of BP_Activity_Activity, that's because the where clauses are "join( ' AND ', $where ) - ed";

So to solve this in 6040_before_4988.03.patch, i've created an array containing all the sql parts and a new filter 'bp_activity_activities_sql_parts' (on a side note, i think people would love to get such an array of sql parts when filtering 'bp_activity_paged_activities_sql': this would avoid preg_matching the query for instance).

Then i was able to filter these parts and use a 'OR' keyword and take benefit of the great work being done in #4988, by using BP_Activity_Query.

Once the patch applied (you'll need to first patch with 6040_before_4988.03.patch and then with 4988.03.patch to avoid too many rejects), what dontdream told about the delete feature is happening:

The != initiator cannot delete the activity

The == initiator can delete the activity.

But we only have 1 public activity, displayed in each user's stream and in the activity directory stream > i like it :)

I think it's not a big deal if the != initiator cannot delete the activity. The real big deal in this case from my point of view is that if the friendship is cancelled by the initiator (or the other one btw), the activity is not deleted so i think we should fix this bug first, reason why i've created #6055.

NB: in 6040_before_4988.03.patch i've included the fix to delete the activities that i'm suggesting in ​6055.patch

[dontdream]

Looks good to me. The single friendship activity is 'owned' by the initiator only and not by both friends, but probably we can live with that.

[boonebgorges]

I think it's not a big deal if the != initiator cannot delete the activity.

I agree with you and dontdream here. Let's go with the single activity item as suggested.

I would strongly prefer to avoid filtering SQL strings to make this work. It's my understanding from #4988 that we can do what imath has suggested here by making sure that 'personal' or 'just-me' activity queries (ie those that populate the user's Personal stream) with the following syntax:

$q = new BP_Activity_Query( array(
    'relation' => 'OR',
    array(
        'field' => 'user_id',
        'value' => $user_id,
    ),
    array(
        'relation' => 'AND',
        array(
            'field' => 'component',
            'value' => 'friends',
        ),
        array(
            'field' => 'secondary_item_id',
            'value' => $user_id,
        ),
    ),
) );

This is the beauty of the nested syntax :)

[imath]

Replying to boonebgorges:

This is the beauty of the nested syntax :)

This is absolutely great!! Just tested and prepared a new patch (6040_after_4988.03.patch) so we will be ready when #4988 patch will be committed. I haven't include the delete bug in this new patch as we now have this ticket #6055 to deal with it.

If you want to test 6040_after_4988.03.patch, make sure ​4988.03.patch has been first applied

[imath]

6040.2.patch is an updated version of the patch taking in account r9256.

I've also added unit tests and an upgrade routine to remove all 'hide_sitewide' activities.

[r-a-y]

Thanks imath for pinging me about this ticket.

Changing the 'just-me' scope to add in activity that wasn't created by that user does not sound like the right way to go about this.

Here's my idea (feel free to shoot it down!):

• Change the second friends_record_activity() type located ​here to 'friendship_accepted' leaving 'hide_sitewide' set to 1. Yes, we would be bringing back 'friendship_accepted'!
• We then do a version of imath's additions, but add the 'friendship_accepted' type to the scope args if the user isn't logged-in. See bp_friends_filter_activity_just_me_scope() in the patch.
• A migration script will be needed for the friendship activity itemsif we do decide to do this.

The other stuff in the patch separates the hardcoded scopes from BP_Activity_Activity::get_scope_query_sql() into separate, filterable scopes similar to how the 'friends' and 'groups' scopes are separated. That way, it would be possible for the friends component to filter the 'just-me' scope to add its special conditions like in the patch.

I haven't added a migration script or unit tests for this yet because I'm not sure if this approach is desired or not.

[boonebgorges]

Changing the 'just-me' scope to add in activity that wasn't created by that user does not sound like the right way to go about this.

This makes sense at a glance, but the more I think about it, the more I think that it's sort of an artificial way of delineating "just-me". "just-me" and the other preset scopes are shorthands that we use to populate specific activity filter tabs. When I look at "my" activity, I want to see activity related to stuff that directly involves me. I don't care whether my used ID is in the 'user_id' column of the database table. Put this way, it makes sense that a single friendship_created activity item would show up in my stream even if I were 'item_id'.

By going with the single activity item, we avoid a bunch of syncing and visibility problems, as described through this ticket. If we can solve these problems in one fell swoop, by combining the duplicate items into a single item, and the only "price" we pay is to change just-me, I think we should do it. I'd be happy to hear arguments to the contrary, though.

[r-a-y]

By going with the single activity item, we avoid a bunch of syncing and visibility problems, as described through this ticket.

Thanks for talking through this.

The more I think about it, the more I'm okay with it. It would be easy to duplicate the Twitter Notifications / Retweet stream if we followed this example.

But, I think separating the hardcoded scopes away from BP_Activity_Activity::get_scope_query_sql() makes sense and allows developers to make adjustments to them as well if needed.

I'm on board!

[imath]

Thanks r-a-y & boonebgorges for your feedbacks

I agree with r-a-y about :

But, I think separating the hardcoded scopes away from BP_Activity_Activity::get_scope_query_sql() makes sense and allows developers to make adjustments to them as well if needed.

So in ​6040.3.patch, i've copied the filters r-a-y created for the different scopes without the changes in src/bp-friends/bp-friends-activity.php. In this file, i've only kept an adapted version of r-a-y's bp_friends_filter_activity_just_me_scope() function. I've also included the upgrade routine and the unittests.

This means, we are only creating one public activity when a friendship is created having the type 'friendship_created'. Since i've followed DJPaul's advice in #6055, only having 'friendship_created' type is making sure activity will be deleted in case a friendship has been removed.

[r-a-y]

imath - 3.patch works in my eyes.

I would split up the commits though. Commit the scope separation first and then the rest of the patch in the next commit.

Thanks for your work on this, imath!

[imath]

Replying to r-a-y:

I would split up the commits though. Commit the scope separation first and then the rest of the patch in the next commit.

Thanks for your feedback r-a-y. I'll try to commit this before JJJ has packaged 2.2 :)

[imath]

In 9325:

Move activity scopes adjustment in filters

Separate the hardcoded scopes away from BP_Activity_Activity::get_scope_query_sql() to manage scope adjustments within components and to allow developers to make adjustments to them if needed.

See #4988
See #6040

props r-a-y

[imath]

In 9326:

Only create a single activity when a friendship is created and make sure to display it on each involved user personal activities.

When a friendship is created, a single public activity will be created. Thanks to the new BP_Activity_Query class introduced in version 2.2, we are able to fetch this activity on each involved user personal activity stream.
Once BuddyPress will be updated to 2.2, all hide_sitewide activities related to a created friendship will be removed as they are not necessary anymore.

Fixes #6040

props boonebgorges, r-a-y