new_blog_post activity items not deleted when blog post is deleted

[henry.wright]

When site tracking is enabled, new blog posts generate an activity entry along the lines of "Member X wrote blog post Hello World".

If the blog post author deletes the blog post, the activity entry remains in the stream - resulting in a broken link to the blog post.

[henry.wright]

Apologies - when I said 'deleted', I should have been more specific. I actually mean 'trashed'.

add_action( 'delete_post', 'bp_blogs_remove_post' ); fires for 'deleted' posts. Just doesn't seem to be a hook that fires for 'trashed' posts.

[boonebgorges]

add_action( 'delete_post', 'bp_blogs_remove_post' ); fires for 'deleted' posts. Just doesn't seem to be a hook that fires for 'trashed' posts.

Ah, if that's true, then it should definitely be fixed. We should think about hooking to 'transition_post_type' instead.

[henry.wright]

Replying to boonebgorges:

We should think about hooking to 'transition_post_type' instead.

agreed - although I'm not familiar with that hook?

My initial thought was to do this:

add_action( 'trashed_post', 'bp_blogs_remove_post' );

But then I thought, wait, what if a post author decides to untrash a post?

So, perhaps a better solution would be to avoid using the bp_blogs_remove_post function when a post is trashed and instead modify the activity item so that it is hidden from the stream.

This can be done easily for the sitewide stream by passing the activity item's ID to bp_activity_add() and setting hide_sitewide => true. Not sure if there is a similar param available which could be used to hide from the user's activity stream?

This general approach could also work for when a post is unpublished and republished etc.

[henry.wright]

Hide

function hide_activity_item( $postid ) {

    $args = array(
        'user_id'           => bp_loggedin_user_id(),
        'type'              => 'new_blog_post',
        'secondary_item_id' => $postid
    );
    $activity_id = bp_activity_get_activity_id( $args );
    
    bp_activity_add( array(
        'id'                => $activity_id,
        'hide_sitewide'     => true
) );
}
add_action( 'trashed_post', 'hide_activity_item' );

And show

function show_activity_item( $postid ) {

    $args = array(
        'user_id'           => bp_loggedin_user_id(),
        'type'              => 'new_blog_post',
        'secondary_item_id' => $postid
    );
    $activity_id = bp_activity_get_activity_id( $args );
    
    bp_activity_add( array(
        'id'                => $activity_id,
        'hide_sitewide'     => false
) );
}
add_action( 'untrashed_post', 'show_activity_item' );

Notes:

1. I haven't tested this bit I'm hoping it illustrates my thoughts.
2. This would only work for the sitewide stream. Is there a hide_user_stream param?

[boonebgorges]

This would only work for the site-wide stream. Is there a hide_user_stream param?

No. hide_sitewide is used for hiding duplicates as well as for privacy purposes. See #3857. It's for this reason that your suggestion, while clever, won't work.

[henry.wright]

Replying to boonebgorges:

No. hide_sitewide is used for hiding duplicates as well as for privacy purposes. See #3857. It's for this reason that your suggestion, while clever, won't work.

I had an idea you were going to say that. Back to the drawing board I go :)

[henry.wright]

Considering ​#3857 seems as though it will be a big task that won't be in place for some time, what are your thoughts on deleting the activity item on trashed_post and adding a new one on untrashed_post? All I can think of is mischievous users might decide this is a good way to 'bump' their post to the top of the activity stream due to a new recorded_time being introduced each time a new activity entry is added.

[boonebgorges]

what are your thoughts on deleting the activity item on trashed_post and adding a new one on untrashed_post?

Yes. Either we should do that, or we should centralize all this logic in bp_blogs_catch_published_post(). See r7577 and #4090.

[henry.wright]

boonebgorges - looks like you are already on to this with bp_blogs_catch_published_post. My vote is let that function house the logic and hook to it where appropriate (as you mentioned in the beginning).

Something like?

if ( 'trash' === $new_status ) { 
    // delete activity item
    return; 
}

Update: On second thoughts please see my patch below which I think is a simpler approach.

[henry.wright]

Please find the attached patch.

I've hooked bp_blogs_remove_post to trashed_post so that activity items are deleted from the activity stream when a post is trashed.

bp_blogs_catch_published_post will detect if a user decides to untrash the post and will handle the creation of a new activity entry.

[henry.wright]

So now my thoughts are turning to "Member X commented on blog post Hello World".

When a blog post is deleted, activity entries such as this are deleted from the activity stream. However, on trashing a post, they're not deleted. So we have the same problem as discussed above.

I'm not sure deleting this sort of activity entry on trashing a post and creating a new activity entry on 'untrash' is a good idea because if a blog post has many comments from a single commenter - that commenter's activity stream is suddenly going to be flooded with many new activity items at the moment the blog post is untrashed.

"Member X commented on blog post Hello World"
"Member X commented on blog post Hello World"
"Member X commented on blog post Hello World"

etc.

This leads me to think a way of hiding and showing activity entries is needed. There's currently no param available for this. Would it be worth introducing one? Or perhaps there is a better way of doing it that I'm missing?

UPDATE:

Completely ignore this particular post.

I was thinking the recorded date of the activity item is the date the activity item is created. Of course this isn't the case. The recorded time is the date the comment was made.

'recorded_time' => $recorded_comment->comment_date_gmt

Hence, the problem I've outlined in this particular post is invalid :)

[DJPaul]

Patch incoming. Spent a few hours looking at this.

• The attached patch works, but probably nicer to integrate into a bp_blogs_catch_published_post()-type wrapper per Boone's comments.
• Also correct that someone could trash and untrash, and as Activity doesn't have an equivalent "trashed" status, so we either have to leave the activity items behind pointing to dead links, or they loose the activity item, per this ticket. :) Consensus seems to be that we should delete activity when a post is trashed.
• I discovered when testing that we don't handle Activity items if the post goes from published -> draft.

[henry.wright]

Hi Paul

My patch doesn't take into account the published-to-draft status transition so it was never a great approach.

Your patch got me thinking of all possible eventualities. Aside from delete, it seems all of the following status transitions can occur:

Published - > Future
Published - > Draft - covered in ​5333-1
Published - > Pending
Published - > Private
Published - > Trash - covered in ​5333-1

​http://codex.wordpress.org/Post_Status

1. Is it worth modifying your patch to take all these possibilities into account?
2. Should the same housekeeping be done for blog post comments?

[boonebgorges]

DJPaul - Thanks for the patch.

but probably nicer to integrate into a bp_blogs_catch_published_post()-type wrapper

Yup, the more possibilities I'm seeing, the more I'm thinking it should all be centralized.

See 5333.02.patch.

In other words:

1. Do nothing if this is just an edit.
2. If the post is being published (ie, transitioned to 'publish' from a non-'publish' status), post a new activity item. (This reproduces the existing bp_blogs_catch_published_post().)
3. If the post is being unpublished, delete the activity item. DJPaul, your patch only deletes for publish->trash and publish->draft, but I think henry.wright is correct that we should account for other cases as well. Was your original concern about custom statuses? Maybe we can just extend our whitelist.

Should the same housekeeping be done for blog post comments?

Yes. Let's wait until we get it how we want it for posts, and then we can port it to comments.

[henry.wright]

I like how you've used if ( 'publish' === $old_status ) to catch everything! Very nice and concise!

Yes. Let's wait until we get it how we want it for posts, and then we can port it to comments.

OK, sounds good.

[DJPaul]

Looks good

[boonebgorges]

In 8073:

Improved management of new_blog_post activity items at transition_post_status

Introduces bp_blogs_catch_transition_post_status(). This function, which
replaces bp_blogs_catch_published_post(), performs a similar task - determining
when a new blog post has been published, and recording the post in the activity
stream - but adds a corrolary for unpublishing that deletes the activity item.

Unit tests and deprecated function are included.

See #5333

Props henry.wright for an initial patch

[boonebgorges]

henry.wright - I'm leaving this ticket open in case you feel like writing a similar function (and ideally unit tests!) for comment status transitions. Thanks!

[r-a-y]

In 8191:

Blogs: Tweak what happens when a post status is changed.

r8073 introduced bp_blogs_catch_transition_post_status() to determine
how blog posts are recorded in the activity stream. However, blog
post edits are not accounted for.

This commit:

• Updates a blog post's activity item when a post is edited
• Mirrors a blog post's comment status to activitymeta for use with bp_blogs_comments_open() (see r8189)
• Removes the usage of bp_blogs_record_post() when a post has changed to an unpublished status. bp_blogs_record_post() already fires on the 'delete_post' action, so this isn't necessary. Instead, we remove the corresponding activity entry.

See #5333, #5130

[boonebgorges]

Going to mark this ticket as resolved to clear the milestone. I think most of the necessary work for comments is already done, but if there are additional issues, let's tackle them in separate tickets for 2.1.